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

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/models/concerns/issuable.rb

@@ -255,8 +255,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)]
@@ -270,8 +272,12 @@ module Issuable
       end
     end
 
-    if old_total_time_spent != total_time_spent
-      changes[: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)

app/services/issuable_base_service.rb

@@ -169,10 +169,7 @@ class IssuableBaseService < BaseService
     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
+    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)
@@ -193,18 +190,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)
@@ -212,9 +204,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
@@ -267,6 +258,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

@@ -4,8 +4,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
@@ -14,9 +14,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

@@ -22,8 +22,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

@@ -283,7 +283,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
 
@@ -302,7 +302,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
 
@@ -322,7 +322,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
 
@@ -345,7 +345,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

@@ -65,7 +65,7 @@ describe MergeRequests::UpdateService, :mailer do
         end
       end
 
-      it 'mathces base expectations' do
+      it 'matches base expectations' do
         expect(@merge_request).to be_valid
         expect(@merge_request.title).to eq('New title')
         expect(@merge_request.assignee).to eq(user2)
@@ -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