Merge branch '40226-refactor-the-issuable-s-webhooks-data-architecture-ee' into 'master'

[EE] Refactor the way we pass `old associations` to issuable's update services

See merge request gitlab-org/gitlab-ee!3537

app/models/concerns/issuable.rb

@@ -265,8 +265,10 @@ module Issuable
     participants(user).include?(user)
   end
 
-  def to_hook_data(user, old_labels: [], old_assignees: [], old_total_time_spent: nil)
+  def to_hook_data(user, old_associations: {})
     changes = previous_changes
+    old_labels = old_associations.fetch(:labels, [])
+    old_assignees = old_associations.fetch(:assignees, [])
 
     if old_labels != labels
       changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
@@ -280,9 +282,13 @@ module Issuable
       end
     end
 
-    if self.respond_to?(:total_time_spent) && old_total_time_spent != total_time_spent
+    if self.respond_to?(:total_time_spent)
+      old_total_time_spent = old_associations.fetch(:total_time_spent, nil)
+
+      if old_total_time_spent != total_time_spent
         changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
       end
+    end
 
     Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
   end

app/services/issuable_base_service.rb

@@ -165,16 +165,13 @@ class IssuableBaseService < BaseService
     # To be overridden by subclasses
   end
 
-  def update(issuable) # rubocop:disable Metrics/AbcSize
+  def update(issuable)
     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
-    old_assignees = issuable.assignees.to_a
-    old_total_time_spent = issuable.total_time_spent if issuable.respond_to?(:total_time_spent)
+    old_associations = associations_before_update(issuable)
 
     label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
     params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
@@ -195,18 +192,13 @@ class IssuableBaseService < BaseService
       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
-          Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels)
+          Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_associations[:labels])
         end
 
-        handle_changes(
-          issuable,
-          old_labels: old_labels,
-          old_mentioned_users: old_mentioned_users,
-          old_assignees: old_assignees
-        )
+        handle_changes(issuable, old_associations: old_associations)
 
         new_assignees = issuable.assignees.to_a
-        affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees)
+        affected_assignees = (old_associations[:assignees] + new_assignees) - (old_associations[:assignees] & new_assignees)
 
         invalidate_cache_counts(issuable, users: affected_assignees.compact)
         after_update(issuable)
@@ -214,9 +206,8 @@ class IssuableBaseService < BaseService
         execute_hooks(
           issuable,
           'update',
-          old_labels: old_labels,
-          old_assignees: old_assignees,
-          old_total_time_spent: old_total_time_spent)
+          old_associations: old_associations
+        )
 
         issuable.update_project_counter_caches if update_project_counters
       end
@@ -269,6 +260,18 @@ class IssuableBaseService < BaseService
     end
   end
 
+  def associations_before_update(issuable)
+    associations =
+      {
+        labels: issuable.labels.to_a,
+        mentioned_users: issuable.mentioned_users.to_a,
+        assignees: issuable.assignees.to_a
+      }
+    associations[:total_time_spent] = issuable.total_time_spent if issuable.respond_to?(:total_time_spent)
+
+    associations
+  end
+
   def has_changes?(issuable, old_labels: [], old_assignees: [])
     valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch]
 

app/services/issues/base_service.rb

 module Issues
   class BaseService < ::IssuableBaseService
-    def hook_data(issue, action, old_labels: [], old_assignees: [], old_total_time_spent: nil)
-      hook_data = issue.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees, old_total_time_spent: old_total_time_spent)
+    def hook_data(issue, action, old_associations: {})
+      hook_data = issue.to_hook_data(current_user, old_associations: old_associations)
       hook_data[:object_attributes][:action] = action
 
       hook_data
@@ -22,8 +22,8 @@ module Issues
         issue, issue.project, current_user, old_assignees)
     end
 
-    def execute_hooks(issue, action = 'open', old_labels: [], old_assignees: [], old_total_time_spent: nil)
-      issue_data  = hook_data(issue, action, old_labels: old_labels, old_assignees: old_assignees, old_total_time_spent: old_total_time_spent)
+    def execute_hooks(issue, action = 'open', old_associations: {})
+      issue_data  = hook_data(issue, action, old_associations: old_associations)
       hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
       issue.project.execute_hooks(issue_data, hooks_scope)
       issue.project.execute_services(issue_data, hooks_scope)

app/services/issues/update_service.rb

@@ -14,9 +14,10 @@ module Issues
     end
 
     def handle_changes(issue, options)
-      old_labels = options[:old_labels] || []
-      old_mentioned_users = options[:old_mentioned_users] || []
-      old_assignees = options[:old_assignees] || []
+      old_associations = options.fetch(:old_associations, {})
+      old_labels = old_associations.fetch(:labels, [])
+      old_mentioned_users = old_associations.fetch(:mentioned_users, [])
+      old_assignees = old_associations.fetch(:assignees, [])
 
       if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees)
         todo_service.mark_pending_todos_as_done(issue, current_user)

app/services/merge_requests/base_service.rb

@@ -6,8 +6,8 @@ module MergeRequests
       SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil)
     end
 
-    def hook_data(merge_request, action, old_rev: nil, old_labels: [], old_assignees: [], old_total_time_spent: nil)
-      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees, old_total_time_spent: old_total_time_spent)
+    def hook_data(merge_request, action, old_rev: nil, old_associations: {})
+      hook_data = merge_request.to_hook_data(current_user, old_associations: old_associations)
       hook_data[:object_attributes][:action] = action
       if old_rev && !Gitlab::Git.blank_ref?(old_rev)
         hook_data[:object_attributes][:oldrev] = old_rev
@@ -16,9 +16,9 @@ module MergeRequests
       hook_data
     end
 
-    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [], old_assignees: [], old_total_time_spent: nil)
+    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_associations: {})
       if merge_request.project
-        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels, old_assignees: old_assignees, old_total_time_spent: old_total_time_spent)
+        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_associations: old_associations)
         merge_request.project.execute_hooks(merge_data, :merge_request_hooks)
         merge_request.project.execute_services(merge_data, :merge_request_hooks)
       end

app/services/merge_requests/update_service.rb

@@ -33,8 +33,9 @@ module MergeRequests
     end
 
     def handle_changes(merge_request, options)
-      old_labels = options[:old_labels] || []
-      old_mentioned_users = options[:old_mentioned_users] || []
+      old_associations = options.fetch(:old_associations, {})
+      old_labels = old_associations.fetch(:labels, [])
+      old_mentioned_users = old_associations.fetch(:mentioned_users, [])
 
       if has_changes?(merge_request, old_labels: old_labels)
         todo_service.mark_pending_todos_as_done(merge_request, current_user)

spec/models/concerns/issuable_spec.rb

@@ -300,7 +300,7 @@ describe Issuable do
             'labels' => [[labels[0].hook_attrs], [labels[1].hook_attrs]]
           ))
 
-        issue.to_hook_data(user, old_labels: [labels[0]])
+        issue.to_hook_data(user, old_associations: { labels: [labels[0]] })
       end
     end
 
@@ -319,7 +319,7 @@ describe Issuable do
             'total_time_spent' => [1, 2]
           ))
 
-        issue.to_hook_data(user, old_total_time_spent: 1)
+        issue.to_hook_data(user, old_associations: { total_time_spent: 1 })
       end
     end
 
@@ -339,7 +339,7 @@ describe Issuable do
             'assignees' => [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]]
           ))
 
-        issue.to_hook_data(user, old_assignees: [user])
+        issue.to_hook_data(user, old_associations: { assignees: [user] })
       end
     end
 
@@ -362,7 +362,7 @@ describe Issuable do
             'assignee' => [user.hook_attrs, user2.hook_attrs]
           ))
 
-        merge_request.to_hook_data(user, old_assignees: [user])
+        merge_request.to_hook_data(user, old_associations: { assignees: [user] })
       end
     end
   end

spec/services/merge_requests/update_service_spec.rb

@@ -78,9 +78,17 @@ describe MergeRequests::UpdateService, :mailer do
       end
 
       it 'executes hooks with update action' do
-        expect(service)
-          .to have_received(:execute_hooks)
-                .with(@merge_request, 'update', old_labels: [], old_assignees: [user3], old_total_time_spent: 0)
+        expect(service).to have_received(:execute_hooks)
+          .with(
+            @merge_request,
+            'update',
+            old_associations: {
+              labels: [],
+              mentioned_users: [user2],
+              assignees: [user3],
+              total_time_spent: 0
+            }
+          )
       end
 
       it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do