Commit 075d6516 authored by Rémy Coutable's avatar Rémy Coutable

Start adding Gitlab::HookData::IssuableBuilder

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 67d5ca9f
...@@ -256,29 +256,22 @@ module Issuable ...@@ -256,29 +256,22 @@ module Issuable
participants(user).include?(user) participants(user).include?(user)
end end
def to_hook_data(user, old_labels: []) def to_hook_data(user, old_labels: [], old_assignees: [])
changes = previous_changes changes = previous_changes
if old_labels != labels if old_labels != labels
changes[:labels] = [old_labels.map(&:name), labels.map(&:name)] changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
end end
hook_data = { if old_assignees != assignees
object_kind: self.class.name.underscore, if self.is_a?(Issue)
user: user.hook_attrs, changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
project: project.hook_attrs, else
object_attributes: hook_attrs, changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
labels: labels.map(&:hook_attrs), end
changes: changes,
# DEPRECATED
repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
}
if self.is_a?(Issue)
hook_data[:assignees] = assignees.map(&:hook_attrs) if assignees.any?
else
hook_data[:assignee] = assignee.hook_attrs if assignee
end end
hook_data Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
end end
def labels_array def labels_array
......
...@@ -18,6 +18,36 @@ class Issue < ActiveRecord::Base ...@@ -18,6 +18,36 @@ class Issue < ActiveRecord::Base
DueThisWeek = DueDateStruct.new('Due This Week', 'week').freeze DueThisWeek = DueDateStruct.new('Due This Week', 'week').freeze
DueThisMonth = DueDateStruct.new('Due This Month', 'month').freeze DueThisMonth = DueDateStruct.new('Due This Month', 'month').freeze
SAFE_HOOK_ATTRIBUTES = %i[
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
].freeze
SAFE_HOOK_RELATIONS = %i[
assignees
labels
].freeze
belongs_to :project belongs_to :project
belongs_to :moved_to, class_name: 'Issue' belongs_to :moved_to, class_name: 'Issue'
...@@ -74,21 +104,6 @@ class Issue < ActiveRecord::Base ...@@ -74,21 +104,6 @@ class Issue < ActiveRecord::Base
end end
end end
def hook_attrs
assignee_ids = self.assignee_ids
attrs = {
url: Gitlab::UrlBuilder.build(self),
total_time_spent: total_time_spent,
human_total_time_spent: human_total_time_spent,
human_time_estimate: human_time_estimate,
assignee_ids: assignee_ids,
assignee_id: assignee_ids.first # This key is deprecated
}
attributes.merge!(attrs)
end
def self.reference_prefix def self.reference_prefix
'#' '#'
end end
...@@ -132,6 +147,30 @@ class Issue < ActiveRecord::Base ...@@ -132,6 +147,30 @@ class Issue < ActiveRecord::Base
"id DESC") "id DESC")
end end
def self.safe_hook_attributes
SAFE_HOOK_ATTRIBUTES
end
def self.safe_hook_relations
SAFE_HOOK_RELATIONS
end
def hook_attrs
assignee_ids = self.assignee_ids
attrs = {
url: Gitlab::UrlBuilder.build(self),
total_time_spent: total_time_spent,
human_total_time_spent: human_total_time_spent,
human_time_estimate: human_time_estimate,
assignee_ids: assignee_ids,
assignee_id: assignee_ids.first # This key is deprecated
}
attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
.merge!(attrs)
end
# Returns a Hash of attributes to be used for Twitter card metadata # Returns a Hash of attributes to be used for Twitter card metadata
def card_attributes def card_attributes
{ {
......
...@@ -7,6 +7,41 @@ class MergeRequest < ActiveRecord::Base ...@@ -7,6 +7,41 @@ class MergeRequest < ActiveRecord::Base
include IgnorableColumn include IgnorableColumn
include CreatedAtFilterable include CreatedAtFilterable
SAFE_HOOK_ATTRIBUTES = %i[
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
].freeze
SAFE_HOOK_RELATIONS = %i[
assignee
labels
].freeze
ignore_column :locked_at ignore_column :locked_at
belongs_to :target_project, class_name: "Project" belongs_to :target_project, class_name: "Project"
...@@ -179,6 +214,30 @@ class MergeRequest < ActiveRecord::Base ...@@ -179,6 +214,30 @@ class MergeRequest < ActiveRecord::Base
work_in_progress?(title) ? title : "WIP: #{title}" work_in_progress?(title) ? title : "WIP: #{title}"
end end
def self.safe_hook_attributes
SAFE_HOOK_ATTRIBUTES
end
def self.safe_hook_relations
SAFE_HOOK_RELATIONS
end
def hook_attrs
attrs = {
url: Gitlab::UrlBuilder.build(self),
source: source_project.try(:hook_attrs),
target: target_project.hook_attrs,
last_commit: diff_head_commit&.hook_attrs,
work_in_progress: work_in_progress?,
total_time_spent: total_time_spent,
human_total_time_spent: human_total_time_spent,
human_time_estimate: human_time_estimate
}
attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
.merge!(attrs)
end
# Returns a Hash of attributes to be used for Twitter card metadata # Returns a Hash of attributes to be used for Twitter card metadata
def card_attributes def card_attributes
{ {
...@@ -587,25 +646,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -587,25 +646,6 @@ class MergeRequest < ActiveRecord::Base
!discussions_to_be_resolved? !discussions_to_be_resolved?
end end
def hook_attrs
attrs = {
url: Gitlab::UrlBuilder.build(self),
source: source_project.try(:hook_attrs),
target: target_project.hook_attrs,
last_commit: nil,
work_in_progress: work_in_progress?,
total_time_spent: total_time_spent,
human_total_time_spent: human_total_time_spent,
human_time_estimate: human_time_estimate
}
if diff_head_commit
attrs[:last_commit] = diff_head_commit.hook_attrs
end
attributes.merge!(attrs)
end
def for_fork? def for_fork?
target_project != source_project target_project != source_project
end end
......
...@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService ...@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService
invalidate_cache_counts(issuable, users: affected_assignees.compact) invalidate_cache_counts(issuable, users: affected_assignees.compact)
after_update(issuable) after_update(issuable)
issuable.create_new_cross_references!(current_user) issuable.create_new_cross_references!(current_user)
execute_hooks(issuable, 'update', old_labels: old_labels) execute_hooks(issuable, 'update', old_labels: old_labels, old_assignees: old_assignees)
issuable.update_project_counter_caches if update_project_counters issuable.update_project_counter_caches if update_project_counters
end end
......
module Issues module Issues
class BaseService < ::IssuableBaseService class BaseService < ::IssuableBaseService
def hook_data(issue, action, old_labels: []) def hook_data(issue, action, old_labels: [], old_assignees: [])
hook_data = issue.to_hook_data(current_user, old_labels: old_labels) hook_data = issue.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
hook_data[:object_attributes][:action] = action hook_data[:object_attributes][:action] = action
hook_data hook_data
...@@ -22,8 +22,8 @@ module Issues ...@@ -22,8 +22,8 @@ module Issues
issue, issue.project, current_user, old_assignees) issue, issue.project, current_user, old_assignees)
end end
def execute_hooks(issue, action = 'open', old_labels: []) def execute_hooks(issue, action = 'open', old_labels: [], old_assignees: [])
issue_data = hook_data(issue, action, old_labels: old_labels) issue_data = hook_data(issue, action, old_labels: old_labels, old_assignees: old_assignees)
hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
issue.project.execute_hooks(issue_data, hooks_scope) issue.project.execute_hooks(issue_data, hooks_scope)
issue.project.execute_services(issue_data, hooks_scope) issue.project.execute_services(issue_data, hooks_scope)
......
...@@ -18,8 +18,8 @@ module MergeRequests ...@@ -18,8 +18,8 @@ module MergeRequests
super if changed_title super if changed_title
end end
def hook_data(merge_request, action, old_rev: nil, old_labels: []) 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) hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
hook_data[:object_attributes][:action] = action hook_data[:object_attributes][:action] = action
if old_rev && !Gitlab::Git.blank_ref?(old_rev) if old_rev && !Gitlab::Git.blank_ref?(old_rev)
hook_data[:object_attributes][:oldrev] = old_rev hook_data[:object_attributes][:oldrev] = old_rev
...@@ -28,9 +28,9 @@ module MergeRequests ...@@ -28,9 +28,9 @@ module MergeRequests
hook_data hook_data
end end
def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: []) def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [], old_assignees: [])
if merge_request.project if merge_request.project
merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels) merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels, old_assignees: old_assignees)
merge_request.project.execute_hooks(merge_data, :merge_request_hooks) merge_request.project.execute_hooks(merge_data, :merge_request_hooks)
merge_request.project.execute_services(merge_data, :merge_request_hooks) merge_request.project.execute_services(merge_data, :merge_request_hooks)
end end
......
module Gitlab
module HookData
class IssuableBuilder
attr_accessor :issuable
def initialize(issuable)
@issuable = issuable
end
def build(user: nil, changes: {})
hook_data = {
object_kind: issuable.class.name.underscore,
user: user.hook_attrs,
project: issuable.project.hook_attrs,
object_attributes: issuable.hook_attrs,
labels: issuable.labels.map(&:hook_attrs),
changes: changes.slice(*safe_keys),
# DEPRECATED
repository: issuable.project.hook_attrs.slice(:name, :url, :description, :homepage)
}
if issuable.is_a?(Issue)
hook_data[:assignees] = issuable.assignees.map(&:hook_attrs) if issuable.assignees.any?
else
hook_data[:assignee] = issuable.assignee.hook_attrs if issuable.assignee
end
hook_data
end
def safe_keys
issuable.class.safe_hook_attributes + issuable.class.safe_hook_relations
end
end
end
end
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
...@@ -264,41 +264,55 @@ describe Issuable do ...@@ -264,41 +264,55 @@ describe Issuable do
end end
end end
describe "#to_hook_data" do describe '#to_hook_data' do
it_behaves_like 'issuable hook data', 'issue' do context 'labels are updated' do
let(:issuable) { create(:issue, description: 'A description') } let(:labels) { create_list(:label, 2) }
end let(:data) { issue.to_hook_data(user, old_labels: [labels[0]]) }
it_behaves_like 'issuable hook data', 'merge_request' do before do
let(:issuable) { create(:merge_request, description: 'A description') } 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 end
context "issue is assigned" do context 'issue is assigned' do
let(:data) { issue.to_hook_data(user) } let(:user2) { create(:user) }
let(:data) { issue.to_hook_data(user, old_assignees: [user]) }
before do before do
issue.assignees << user issue.assignees << user << user2
end end
it "returns correct hook data" do it 'returns correct hook data' do
expect(data[:assignees].first).to eq(user.hook_attrs) 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
end end
context "merge_request is assigned" do context 'merge_request is assigned' do
let(:merge_request) { create(:merge_request) } 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 before do
merge_request.update_attribute(:assignee, user) merge_request.update(assignee: user)
merge_request.update(assignee: user2)
end end
it "returns correct hook data" do it 'returns correct hook data' do
expect(data[:object_attributes]['assignee_id']).to eq(user.id) expect(data[:object_attributes]['assignee_id']).to eq(user2.id)
expect(data[:assignee]).to eq(user.hook_attrs) expect(data[:assignee]).to eq(user2.hook_attrs)
expect(data[:changes]).to match(hash_including({ expect(data[:changes]).to match(hash_including({
'assignee_id' => [nil, user.id], assignee_id: [user.id, user2.id],
'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)] assignee: [user.hook_attrs, user2.hook_attrs]
})) }))
end end
end end
......
...@@ -702,15 +702,39 @@ describe Issue do ...@@ -702,15 +702,39 @@ describe Issue do
describe '#hook_attrs' do describe '#hook_attrs' do
let(:attrs_hash) { subject.hook_attrs } 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(:total_time_spent)
expect(attrs_hash).to include(:human_time_estimate) expect(attrs_hash).to include(:human_time_estimate)
expect(attrs_hash).to include(:human_total_time_spent) 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) expect(attrs_hash).to include(:assignee_ids)
end end
end end
......
...@@ -660,19 +660,53 @@ describe MergeRequest do ...@@ -660,19 +660,53 @@ describe MergeRequest do
end end
end end
describe "#hook_attrs" do describe '#hook_attrs' do
let(:attrs_hash) { subject.hook_attrs } 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 describe "#{key} key" do
include_examples 'project hook data', project_key: key do include_examples 'project hook data', project_key: key do
let(:data) { attrs_hash } let(:data) { attrs_hash }
let(:project) { subject.send("#{key}_project") } let(:project) { subject.public_send("#{key}_project") }
end end
end 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(:source)
expect(attrs_hash).to include(:target) expect(attrs_hash).to include(:target)
expect(attrs_hash).to include(:last_commit) expect(attrs_hash).to include(:last_commit)
...@@ -680,7 +714,6 @@ describe MergeRequest do ...@@ -680,7 +714,6 @@ describe MergeRequest do
expect(attrs_hash).to include(:total_time_spent) expect(attrs_hash).to include(:total_time_spent)
expect(attrs_hash).to include(:human_time_estimate) expect(attrs_hash).to include(:human_time_estimate)
expect(attrs_hash).to include(:human_total_time_spent) expect(attrs_hash).to include(:human_total_time_spent)
expect(attrs_hash).to include('time_estimate')
end end
end end
......
...@@ -78,8 +78,9 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -78,8 +78,9 @@ describe MergeRequests::UpdateService, :mailer do
end end
it 'executes hooks with update action' do it 'executes hooks with update action' do
expect(service).to have_received(:execute_hooks) expect(service)
.with(@merge_request, 'update', old_labels: []) .to have_received(:execute_hooks)
.with(@merge_request, 'update', old_labels: [], old_assignees: [user3])
end end
it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment