Refactor the way we pass `old associations` to issuable's update services40226-refactor-the-issuable-s-webhooks-data-architectur

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

8 files changed, 58 insertions, 39 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 35090181bd9..f4cbdc9d88f 100644
--- a/app/models/concerns/issuable.rb
+++ b/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)
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 39a7299ff60..92215dd25bb 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -163,16 +163,25 @@ class IssuableBaseService < BaseService
     # To be overridden by subclasses
   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 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
+    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 +202,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 +216,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
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 0f711bcc3cf..9f6cfc0f6d3 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,7 +1,7 @@
 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)
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 1b7b5927c5a..d7aa7e2347e 100644
--- a/app/services/issues/update_service.rb
+++ b/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)
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index d3938b065bc..542d95cc4a9 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -18,8 +18,8 @@ module MergeRequests
       super if changed_title
     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
@@ -28,9 +28,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
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 1f394cacc64..c153872c874 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/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)
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 4dfbb14952e..765b2729918 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/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
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 5ce6ca70c83..7a66b809550 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/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