Start adding Gitlab::HookData::IssuableBuilder

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

6 files changed, 201 insertions, 90 deletions

diff --git a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb
new file mode 100644
index 00000000000..31cc5eaea88
--- /dev/null
+++ b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb
@@ -0,0 +1,97 @@
+require 'spec_helper'
+
+describe Gitlab::HookData::IssuableBuilder do
+  set(:user) { create(:user) }
+
+  # This shared example requires a `builder` and `user` variable
+  shared_examples 'issuable hook data' do |kind|
+    let(:data) { builder.build(user: user) }
+
+    include_examples 'project hook data' do
+      let(:project) { builder.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[:project]).to eq(builder.issuable.project.hook_attrs)
+        expect(data[:object_attributes]).to eq(builder.issuable.hook_attrs)
+        expect(data[:changes]).to eq({})
+        expect(data[:repository]).to eq(builder.issuable.project.hook_attrs.slice(:name, :url, :description, :homepage))
+      end
+
+      it 'does not contain certain keys' do
+        expect(data).not_to have_key(:assignees)
+        expect(data).not_to have_key(:assignee)
+      end
+
+      describe 'changes are given' do
+        let(:changes) do
+          {
+            cached_markdown_version: %w[foo bar],
+            description: ['A description', 'A cool description'],
+            description_html: %w[foo bar],
+            in_progress_merge_commit_sha: %w[foo bar],
+            lock_version: %w[foo bar],
+            merge_jid: %w[foo bar],
+            title: ['A title', 'Hello World'],
+            title_html: %w[foo bar]
+          }
+        end
+        let(:data) { builder.build(user: user, changes: changes) }
+
+        it 'populates the :changes hash' do
+          expect(data[:changes]).to match(hash_including({
+            title: ['A title', 'Hello World'],
+            description: ['A description', 'A cool description']
+          }))
+        end
+
+        it 'does not contain certain keys' do
+          expect(data[:changes]).not_to have_key('cached_markdown_version')
+          expect(data[:changes]).not_to have_key('description_html')
+          expect(data[:changes]).not_to have_key('lock_version')
+          expect(data[:changes]).not_to have_key('title_html')
+          expect(data[:changes]).not_to have_key('in_progress_merge_commit_sha')
+          expect(data[:changes]).not_to have_key('merge_jid')
+        end
+      end
+    end
+  end
+
+  describe '#build' do
+    it_behaves_like 'issuable hook data', 'issue' do
+      let(:issuable) { create(:issue, description: 'A description') }
+      let(:builder) { described_class.new(issuable) }
+    end
+
+    it_behaves_like 'issuable hook data', 'merge_request' do
+      let(:issuable) { create(:merge_request, description: 'A description') }
+      let(:builder) { described_class.new(issuable) }
+    end
+
+    context 'issue is assigned' do
+      let(:issue) { create(:issue).tap { |i| i.assignees << user } }
+      let(:data) { described_class.new(issue).build(user: user) }
+
+      it 'returns correct hook data' do
+        expect(data[:object_attributes]['assignee_id']).to eq(user.id)
+        expect(data[:assignees].first).to eq(user.hook_attrs)
+        expect(data).not_to have_key(:assignee)
+      end
+    end
+
+    context 'merge_request is assigned' do
+      let(:merge_request) { create(:merge_request).tap { |mr| mr.update(assignee: user) } }
+      let(:data) { described_class.new(merge_request).build(user: user) }
+
+      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).not_to have_key(:assignees)
+      end
+    end
+  end
+end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 5763e1ba6d4..1f8541a3262 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -264,41 +264,55 @@ describe Issuable do
     end
   end
 
-  describe "#to_hook_data" do
-    it_behaves_like 'issuable hook data', 'issue' do
-      let(:issuable) { create(:issue, description: 'A description') }
-    end
+  describe '#to_hook_data' do
+    context 'labels are updated' do
+      let(:labels) { create_list(:label, 2) }
+      let(:data) { issue.to_hook_data(user, old_labels: [labels[0]]) }
 
-    it_behaves_like 'issuable hook data', 'merge_request' do
-      let(:issuable) { create(:merge_request, description: 'A description') }
+      before do
+        issue.update(labels: [labels[1]])
+      end
+
+      it 'includes labels in the hook data' do
+        expect(data[:labels]).to eq([labels[1].hook_attrs])
+        expect(data[:changes]).to match(hash_including({
+          labels: [[labels[0].hook_attrs], [labels[1].hook_attrs]]
+        }))
+      end
     end
 
-    context "issue is assigned" do
-      let(:data) { issue.to_hook_data(user) }
+    context 'issue is assigned' do
+      let(:user2) { create(:user) }
+      let(:data) { issue.to_hook_data(user, old_assignees: [user]) }
 
       before do
-        issue.assignees << user
+        issue.assignees << user << user2
       end
 
-      it "returns correct hook data" do
-        expect(data[:assignees].first).to eq(user.hook_attrs)
+      it 'returns correct hook data' do
+        expect(data[:assignees]).to eq([user.hook_attrs, user2.hook_attrs])
+        expect(data[:changes]).to match(hash_including({
+          assignees: [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]]
+        }))
       end
     end
 
-    context "merge_request is assigned" do
+    context 'merge_request is assigned' do
       let(:merge_request) { create(:merge_request) }
-      let(:data) { merge_request.to_hook_data(user) }
+      let(:user2) { create(:user) }
+      let(:data) { merge_request.to_hook_data(user, old_assignees: [user]) }
 
       before do
-        merge_request.update_attribute(:assignee, user)
+        merge_request.update(assignee: user)
+        merge_request.update(assignee: user2)
       end
 
-      it "returns correct hook data" do
-        expect(data[:object_attributes]['assignee_id']).to eq(user.id)
-        expect(data[:assignee]).to eq(user.hook_attrs)
+      it 'returns correct hook data' do
+        expect(data[:object_attributes]['assignee_id']).to eq(user2.id)
+        expect(data[:assignee]).to eq(user2.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)]
+          assignee_id: [user.id, user2.id],
+          assignee: [user.hook_attrs, user2.hook_attrs]
         }))
       end
     end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index e547da0cfbe..bd1ae3c4945 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -702,15 +702,39 @@ describe Issue do
   describe '#hook_attrs' do
     let(:attrs_hash) { subject.hook_attrs }
 
-    it 'includes time tracking attrs' do
+    it 'includes safe attribute' do
+      %w[
+        assignee_id
+        author_id
+        branch_name
+        closed_at
+        confidential
+        created_at
+        deleted_at
+        description
+        due_date
+        id
+        iid
+        last_edited_at
+        last_edited_by_id
+        milestone_id
+        moved_to_id
+        project_id
+        relative_position
+        state
+        time_estimate
+        title
+        updated_at
+        updated_by_id
+      ].each do |key|
+        expect(attrs_hash).to include(key)
+      end
+    end
+
+    it 'includes additional attrs' do
       expect(attrs_hash).to include(:total_time_spent)
       expect(attrs_hash).to include(:human_time_estimate)
       expect(attrs_hash).to include(:human_total_time_spent)
-      expect(attrs_hash).to include('time_estimate')
-    end
-
-    it 'includes assignee_ids and deprecated assignee_id' do
-      expect(attrs_hash).to include(:assignee_id)
       expect(attrs_hash).to include(:assignee_ids)
     end
   end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 950af653c80..844ada57e22 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -660,19 +660,53 @@ describe MergeRequest do
     end
   end
 
-  describe "#hook_attrs" do
+  describe '#hook_attrs' do
     let(:attrs_hash) { subject.hook_attrs }
 
-    [:source, :target].each do |key|
+    it 'includes safe attribute' do
+      %w[
+        assignee_id
+        author_id
+        created_at
+        deleted_at
+        description
+        head_pipeline_id
+        id
+        iid
+        last_edited_at
+        last_edited_by_id
+        merge_commit_sha
+        merge_error
+        merge_params
+        merge_status
+        merge_user_id
+        merge_when_pipeline_succeeds
+        milestone_id
+        ref_fetched
+        source_branch
+        source_project_id
+        state
+        target_branch
+        target_project_id
+        time_estimate
+        title
+        updated_at
+        updated_by_id
+      ].each do |key|
+        expect(attrs_hash).to include(key)
+      end
+    end
+
+    %i[source target].each do |key|
       describe "#{key} key" do
         include_examples 'project hook data', project_key: key do
           let(:data)    { attrs_hash }
-          let(:project) { subject.send("#{key}_project") }
+          let(:project) { subject.public_send("#{key}_project") }
         end
       end
     end
 
-    it "has all the required keys" do
+    it 'includes additional attrs' do
       expect(attrs_hash).to include(:source)
       expect(attrs_hash).to include(:target)
       expect(attrs_hash).to include(:last_commit)
@@ -680,7 +714,6 @@ describe MergeRequest do
       expect(attrs_hash).to include(:total_time_spent)
       expect(attrs_hash).to include(:human_time_estimate)
       expect(attrs_hash).to include(:human_total_time_spent)
-      expect(attrs_hash).to include('time_estimate')
     end
   end
 
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 3a8cb4ce83d..7257c359a7e 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -78,8 +78,9 @@ 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: [])
+        expect(service)
+          .to have_received(:execute_hooks)
+                .with(@merge_request, 'update', old_labels: [], old_assignees: [user3])
       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
deleted file mode 100644
index ed1d2f41eae..00000000000
--- a/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb
+++ /dev/null
@@ -1,58 +0,0 @@
-# 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