Include the changes in issuable webhook payloads

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

5 files changed, 78 insertions, 31 deletions

diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index fb5fb7daaab..5763e1ba6d4 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Issuable do
   let(:issuable_class) { Issue }
-  let(:issue) { create(:issue) }
+  let(:issue) { create(:issue, title: 'An issue', description: 'A description') }
   let(:user) { create(:user) }
 
   describe "Associations" do
@@ -265,17 +265,17 @@ describe Issuable do
   end
 
   describe "#to_hook_data" do
-    let(:data) { issue.to_hook_data(user) }
-    let(:project) { issue.project }
+    it_behaves_like 'issuable hook data', 'issue' do
+      let(:issuable) { create(:issue, description: 'A description') }
+    end
 
-    it "returns correct hook data" do
-      expect(data[:object_kind]).to eq("issue")
-      expect(data[:user]).to eq(user.hook_attrs)
-      expect(data[:object_attributes]).to eq(issue.hook_attrs)
-      expect(data).not_to have_key(:assignee)
+    it_behaves_like 'issuable hook data', 'merge_request' do
+      let(:issuable) { create(:merge_request, description: 'A description') }
     end
 
     context "issue is assigned" do
+      let(:data) { issue.to_hook_data(user) }
+
       before do
         issue.assignees << user
       end
@@ -296,23 +296,12 @@ describe Issuable do
       it "returns correct hook data" do
         expect(data[:object_attributes]['assignee_id']).to eq(user.id)
         expect(data[:assignee]).to eq(user.hook_attrs)
+        expect(data[:changes]).to match(hash_including({
+          'assignee_id' => [nil, user.id],
+          'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)]
+        }))
       end
     end
-
-    context 'issue has labels' do
-      let(:labels) { [create(:label), create(:label)] }
-
-      before do
-        issue.update_attribute(:labels, labels)
-      end
-
-      it 'includes labels in the hook data' do
-        expect(data[:labels]).to eq(labels.map(&:hook_attrs))
-      end
-    end
-
-    include_examples 'project hook data'
-    include_examples 'deprecated repository hook data'
   end
 
   describe '#labels_array' do
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 62dbe362ec8..a2c05761f6b 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -61,7 +61,7 @@ describe MergeRequests::RefreshService do
 
       it 'executes hooks with update action' do
         expect(refresh_service).to have_received(:execute_hooks)
-          .with(@merge_request, 'update', @oldrev)
+          .with(@merge_request, 'update', old_rev: @oldrev)
 
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open
@@ -87,7 +87,7 @@ describe MergeRequests::RefreshService do
 
       it 'executes hooks with update action' do
         expect(refresh_service).to have_received(:execute_hooks)
-          .with(@merge_request, 'update', @oldrev)
+          .with(@merge_request, 'update', old_rev: @oldrev)
 
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open
@@ -182,7 +182,7 @@ describe MergeRequests::RefreshService do
 
         it 'executes hooks with update action' do
           expect(refresh_service).to have_received(:execute_hooks)
-            .with(@fork_merge_request, 'update', @oldrev)
+            .with(@fork_merge_request, 'update', old_rev: @oldrev)
 
           expect(@merge_request.notes).to be_empty
           expect(@merge_request).to be_open
@@ -264,7 +264,7 @@ describe MergeRequests::RefreshService do
 
       it 'refreshes the merge request' do
         expect(refresh_service).to receive(:execute_hooks)
-                                       .with(@fork_merge_request, 'update', Gitlab::Git::BLANK_SHA)
+                                       .with(@fork_merge_request, 'update', old_rev: Gitlab::Git::BLANK_SHA)
         allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev)
 
         refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master')
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index b11a1b31f32..3a8cb4ce83d 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -79,7 +79,7 @@ describe MergeRequests::UpdateService, :mailer do
 
       it 'executes hooks with update action' do
         expect(service).to have_received(:execute_hooks)
-                               .with(@merge_request, 'update')
+                               .with(@merge_request, 'update', old_labels: [])
       end
 
       it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do
diff --git a/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb b/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb
new file mode 100644
index 00000000000..ed1d2f41eae
--- /dev/null
+++ b/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb
@@ -0,0 +1,58 @@
+# This shared example requires a `user` variable
+shared_examples 'issuable hook data' do |kind|
+  let(:data) { issuable.to_hook_data(user) }
+
+  include_examples 'project hook data' do
+    let(:project) { issuable.project }
+  end
+  include_examples 'deprecated repository hook data'
+
+  context "with a #{kind}" do
+    it 'contains issuable data' do
+      expect(data[:object_kind]).to eq(kind)
+      expect(data[:user]).to eq(user.hook_attrs)
+      expect(data[:object_attributes]).to eq(issuable.hook_attrs)
+      expect(data[:changes]).to match(hash_including({
+        'author_id' => [nil, issuable.author_id],
+        'cached_markdown_version' => [nil, issuable.cached_markdown_version],
+        'created_at' => [nil, a_kind_of(ActiveSupport::TimeWithZone)],
+        'description' => [nil, issuable.description],
+        'description_html' => [nil, issuable.description_html],
+        'id' => [nil, issuable.id],
+        'iid' => [nil, issuable.iid],
+        'state' => [nil, issuable.state],
+        'title' => [nil, issuable.title],
+        'title_html' => [nil, issuable.title_html],
+        'updated_at' => [nil, a_kind_of(ActiveSupport::TimeWithZone)]
+      }))
+      expect(data).not_to have_key(:assignee)
+    end
+
+    describe 'simple attributes are updated' do
+      before do
+        issuable.update(title: 'Hello World', description: 'A cool description')
+      end
+
+      it 'includes an empty :changes hash' do
+        expect(data[:changes]).to match(hash_including({
+          'title' => [issuable.previous_changes['title'][0], 'Hello World'],
+          'description' => [issuable.previous_changes['description'][0], 'A cool description'],
+          'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)]
+        }))
+      end
+    end
+
+    context "#{kind} has labels" do
+      let(:labels) { [create(:label), create(:label)] }
+
+      before do
+        issuable.update_attribute(:labels, labels)
+      end
+
+      it 'includes labels in the hook data' do
+        expect(data[:labels]).to eq(labels.map(&:hook_attrs))
+        expect(data[:changes]).to eq({ 'labels' => [[], labels.map(&:name)] })
+      end
+    end
+  end
+end
diff --git a/spec/support/project_hook_data_shared_example.rb b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb
index 1eb405d4be8..f0264878811 100644
--- a/spec/support/project_hook_data_shared_example.rb
+++ b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb
@@ -1,4 +1,4 @@
-RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :project|
+shared_examples 'project hook data with deprecateds' do |project_key: :project|
   it 'contains project data' do
     expect(data[project_key][:name]).to eq(project.name)
     expect(data[project_key][:description]).to eq(project.description)
@@ -17,7 +17,7 @@ RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :pro
   end
 end
 
-RSpec.shared_examples 'project hook data' do |project_key: :project|
+shared_examples 'project hook data' do |project_key: :project|
   it 'contains project data' do
     expect(data[project_key][:name]).to eq(project.name)
     expect(data[project_key][:description]).to eq(project.description)
@@ -32,7 +32,7 @@ RSpec.shared_examples 'project hook data' do |project_key: :project|
   end
 end
 
-RSpec.shared_examples 'deprecated repository hook data' do |project_key: :project|
+shared_examples 'deprecated repository hook data' do
   it 'contains deprecated repository data' do
     expect(data[:repository][:name]).to eq(project.name)
     expect(data[:repository][:description]).to eq(project.description)