Merge branch...

Merge branch '40122-only-one-note-webhook-is-triggered-when-a-comment-with-time-spent-is-added' into 'master'

Resolve "Only one note webhook is triggered when a comment with time spent is added"

Closes #40122

See merge request gitlab-org/gitlab-ce!15381

app/models/concerns/issuable.rb

@@ -255,7 +255,7 @@ module Issuable
     participants(user).include?(user)
   end
 
-  def to_hook_data(user, old_labels: [], old_assignees: [])
+  def to_hook_data(user, old_labels: [], old_assignees: [], old_total_time_spent: nil)
     changes = previous_changes
 
     if old_labels != labels
@@ -270,6 +270,10 @@ module Issuable
       end
     end
 
+    if old_total_time_spent != total_time_spent
+      changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
+    end
+
     Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
   end
 

app/services/issuable_base_service.rb

@@ -172,6 +172,7 @@ class IssuableBaseService < BaseService
     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
 
     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)
@@ -208,7 +209,12 @@ class IssuableBaseService < BaseService
         invalidate_cache_counts(issuable, users: affected_assignees.compact)
         after_update(issuable)
         issuable.create_new_cross_references!(current_user)
-        execute_hooks(issuable, 'update', old_labels: old_labels, old_assignees: old_assignees)
+        execute_hooks(
+          issuable,
+          'update',
+          old_labels: old_labels,
+          old_assignees: old_assignees,
+          old_total_time_spent: old_total_time_spent)
 
         issuable.update_project_counter_caches if update_project_counters
       end

app/services/issues/base_service.rb

 module Issues
   class BaseService < ::IssuableBaseService
-    def hook_data(issue, action, old_labels: [], old_assignees: [])
-      hook_data = issue.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
+    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)
       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: [])
-      issue_data  = hook_data(issue, action, old_labels: old_labels, old_assignees: old_assignees)
+    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)
       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/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: [])
-      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
+    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)
       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: [])
+    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [], old_assignees: [], old_total_time_spent: nil)
       if merge_request.project
-        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels, old_assignees: old_assignees)
+        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_request.project.execute_hooks(merge_data, :merge_request_hooks)
         merge_request.project.execute_services(merge_data, :merge_request_hooks)
       end

changelogs/unreleased/40122-only-one-note-webhook-is-triggered-when-a-comment-with-time-spent-is-added.yml

+---
+title: Add total_time_spent to the `changes` hash in issuable Webhook payloads
+merge_request: 15381
+author:
+type: changed

lib/gitlab/hook_data/issue_builder.rb

@@ -28,6 +28,7 @@ module Gitlab
       SAFE_HOOK_RELATIONS = %i[
         assignees
         labels
+        total_time_spent
       ].freeze
 
       attr_accessor :issue

lib/gitlab/hook_data/merge_request_builder.rb

@@ -33,6 +33,7 @@ module Gitlab
       SAFE_HOOK_RELATIONS = %i[
         assignee
         labels
+        total_time_spent
       ].freeze
 
       attr_accessor :merge_request

spec/lib/gitlab/hook_data/issuable_builder_spec.rb

@@ -41,7 +41,8 @@ describe Gitlab::HookData::IssuableBuilder do
             labels: [
               [{ id: 1, title: 'foo' }],
               [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }]
-            ]
+            ],
+            total_time_spent: [1, 2]
           }
         end
         let(:data) { builder.build(user: user, changes: changes) }
@@ -53,6 +54,10 @@ describe Gitlab::HookData::IssuableBuilder do
             labels: {
               previous: [{ id: 1, title: 'foo' }],
               current: [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }]
+            },
+            total_time_spent: {
+              previous: 1,
+              current: 2
             }
           }))
         end

spec/models/concerns/issuable_spec.rb

@@ -265,25 +265,44 @@ describe Issuable do
   end
 
   describe '#to_hook_data' do
+    let(:builder) { double }
+
     context 'labels are updated' do
       let(:labels) { create_list(:label, 2) }
 
       before do
         issue.update(labels: [labels[1]])
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(issue).and_return(builder)
       end
 
       it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
-        builder = double
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'labels' => [[labels[0].hook_attrs], [labels[1].hook_attrs]]
+          ))
 
+        issue.to_hook_data(user, old_labels: [labels[0]])
+      end
+    end
+
+    context 'total_time_spent is updated' do
+      before do
+        issue.spend_time(duration: 2, user: user, spent_at: Time.now)
+        issue.save
         expect(Gitlab::HookData::IssuableBuilder)
           .to receive(:new).with(issue).and_return(builder)
+      end
+
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
         expect(builder).to receive(:build).with(
           user: user,
           changes: hash_including(
-            'labels' => [[labels[0].hook_attrs], [labels[1].hook_attrs]]
+            'total_time_spent' => [1, 2]
           ))
 
-        issue.to_hook_data(user, old_labels: [labels[0]])
+        issue.to_hook_data(user, old_total_time_spent: 1)
       end
     end
 
@@ -292,13 +311,11 @@ describe Issuable do
 
       before do
         issue.assignees << user << user2
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(issue).and_return(builder)
       end
 
       it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
-        builder = double
-
-        expect(Gitlab::HookData::IssuableBuilder)
-          .to receive(:new).with(issue).and_return(builder)
         expect(builder).to receive(:build).with(
           user: user,
           changes: hash_including(
@@ -316,13 +333,11 @@ describe Issuable do
       before do
         merge_request.update(assignee: user)
         merge_request.update(assignee: user2)
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(merge_request).and_return(builder)
       end
 
       it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
-        builder = double
-
-        expect(Gitlab::HookData::IssuableBuilder)
-          .to receive(:new).with(merge_request).and_return(builder)
         expect(builder).to receive(:build).with(
           user: user,
           changes: hash_including(

spec/services/merge_requests/update_service_spec.rb

@@ -80,7 +80,7 @@ describe MergeRequests::UpdateService, :mailer do
       it 'executes hooks with update action' do
         expect(service)
           .to have_received(:execute_hooks)
-                .with(@merge_request, 'update', old_labels: [], old_assignees: [user3])
+                .with(@merge_request, 'update', old_labels: [], old_assignees: [user3], old_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