Commit c01fa530 authored by Douwe Maan's avatar Douwe Maan Committed by Ruben Davila

Merge branch 'fix-confidential-issues-webhooks' into 'master'

Scope webhooks/services that will run for confidential issues

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/20661

See merge request !1986
Conflicts:
	db/schema.rb
parent 96342b8a
......@@ -12,6 +12,7 @@ v 8.11.4
- Creating an issue through our API now emails label subscribers !5720
- Block concurrent updates for Pipeline
- Don't create groups for unallowed users when importing projects
- Scope webhooks/services that will run for confidential issues
- Fix issue boards leak private label names and descriptions
- Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner)
- Remove gitorious. !5866
......
......@@ -13,7 +13,7 @@ module ServiceParams
# `issue_events` and `merge_request_events` (singular!)
# See app/helpers/services_helper.rb for how we
# make those event names plural as special case.
:issues_events, :merge_requests_events,
:issues_events, :confidential_issues_events, :merge_requests_events,
:notify_only_broken_builds, :notify_only_broken_pipelines,
:add_pusher, :send_from_committer_email, :disable_diffs,
:external_wiki_url, :notify, :color,
......
......@@ -59,6 +59,7 @@ class Projects::HooksController < Projects::ApplicationController
:pipeline_events,
:enable_ssl_verification,
:issues_events,
:confidential_issues_events,
:merge_requests_events,
:note_events,
:push_events,
......
......@@ -8,7 +8,9 @@ module ServicesHelper
when "note"
"Event will be triggered when someone adds a comment"
when "issue"
"Event will be triggered when an issue is created/updated/merged"
"Event will be triggered when an issue is created/updated/closed"
when "confidential_issue"
"Event will be triggered when a confidential issue is created/updated/closed"
when "merge_request"
"Event will be triggered when a merge request is created/updated/merged"
when "build"
......@@ -19,7 +21,7 @@ module ServicesHelper
end
def service_event_field_name(event)
event = event.pluralize if %w[merge_request issue].include?(event)
event = event.pluralize if %w[merge_request issue confidential_issue].include?(event)
"#{event}_events"
end
end
......@@ -2,6 +2,7 @@ class ProjectHook < WebHook
belongs_to :project
scope :issue_hooks, -> { where(issues_events: true) }
scope :confidential_issue_hooks, -> { where(confidential_issues_events: true) }
scope :note_hooks, -> { where(note_events: true) }
scope :merge_request_hooks, -> { where(merge_requests_events: true) }
scope :build_hooks, -> { where(build_events: true) }
......
......@@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base
default_value_for :push_events, true
default_value_for :issues_events, false
default_value_for :confidential_issues_events, false
default_value_for :note_events, false
default_value_for :merge_requests_events, false
default_value_for :tag_push_events, false
......
......@@ -39,7 +39,7 @@ class HipchatService < Service
end
def supported_events
%w(push issue merge_request note tag_push build)
%w(push issue confidential_issue merge_request note tag_push build)
end
def execute(data)
......
......@@ -44,7 +44,7 @@ class SlackService < Service
end
def supported_events
%w(push issue merge_request note tag_push build wiki_page)
%w(push issue confidential_issue merge_request note tag_push build wiki_page)
end
def execute(data)
......
......@@ -7,6 +7,7 @@ class Service < ActiveRecord::Base
default_value_for :active, false
default_value_for :push_events, true
default_value_for :issues_events, true
default_value_for :confidential_issues_events, true
default_value_for :merge_requests_events, true
default_value_for :tag_push_events, true
default_value_for :note_events, true
......@@ -33,6 +34,7 @@ class Service < ActiveRecord::Base
scope :push_hooks, -> { where(push_events: true, active: true) }
scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) }
scope :issue_hooks, -> { where(issues_events: true, active: true) }
scope :confidential_issue_hooks, -> { where(confidential_issues_events: true, active: true) }
scope :merge_request_hooks, -> { where(merge_requests_events: true, active: true) }
scope :note_hooks, -> { where(note_events: true, active: true) }
scope :build_hooks, -> { where(build_events: true, active: true) }
......@@ -100,7 +102,7 @@ class Service < ActiveRecord::Base
end
def supported_events
%w(push tag_push issue merge_request wiki_page)
%w(push tag_push issue confidential_issue merge_request wiki_page)
end
def execute(data)
......
......@@ -15,8 +15,9 @@ module Issues
def execute_hooks(issue, action = 'open')
issue_data = hook_data(issue, action)
issue.project.execute_hooks(issue_data, :issue_hooks)
issue.project.execute_services(issue_data, :issue_hooks)
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)
end
end
end
......@@ -3,7 +3,7 @@
.col-md-8.col-lg-7
%strong.light-header= hook.url
%div
- %w(push_events tag_push_events issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger|
- %w(push_events tag_push_events issues_events confidential_issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger|
- if hook.send(trigger)
%span.label.label-gray.deploy-project-label= trigger.titleize
.col-md-4.col-lg-5.text-right-lg.prepend-top-5
......
......@@ -51,6 +51,13 @@
%strong Issues events
%p.light
This URL will be triggered when an issue is created/updated/merged
%li
= f.check_box :confidential_issues_events, class: 'pull-left'
.prepend-left-20
= f.label :confidential_issues_events, class: 'list-label' do
%strong Confidential Issues events
%p.light
This URL will be triggered when a confidential issue is created/updated/merged
%li
= f.check_box :merge_requests_events, class: 'pull-left'
.prepend-left-20
......
class AddConfidentialIssuesEventsToWebHooks < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default :web_hooks, :confidential_issues_events, :boolean, default: false, allow_null: false
end
def down
remove_column :web_hooks, :confidential_issues_events
end
end
class AddConfidentialIssuesEventsToServices < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default :services, :confidential_issues_events, :boolean, default: true, allow_null: false
end
def down
remove_column :services, :confidential_issues_events
end
end
class SetConfidentialIssuesEventsOnWebhooks < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
update_column_in_batches(:web_hooks, :confidential_issues_events, true) do |table, query|
query.where(table[:issues_events].eq(true))
end
end
def down
# noop
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20160819221833) do
ActiveRecord::Schema.define(version: 20160901141443) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -928,6 +928,7 @@ ActiveRecord::Schema.define(version: 20160819221833) do
t.boolean "default", default: false
t.boolean "wiki_page_events", default: true
t.boolean "pipeline_events", default: false, null: false
t.boolean "confidential_issues_events", default: true, null: false
end
add_index "services", ["project_id"], name: "index_services_on_project_id", using: :btree
......@@ -1143,6 +1144,7 @@ ActiveRecord::Schema.define(version: 20160819221833) do
t.boolean "wiki_page_events", default: false, null: false
t.string "token"
t.boolean "pipeline_events", default: false, null: false
t.boolean "confidential_issues_events", default: false, null: false
end
add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
......
......@@ -18,12 +18,12 @@ describe Issues::CloseService, services: true do
context "valid params" do
before do
perform_enqueued_jobs do
@issue = described_class.new(project, user, {}).execute(issue)
described_class.new(project, user).execute(issue)
end
end
it { expect(@issue).to be_valid }
it { expect(@issue).to be_closed }
it { expect(issue).to be_valid }
it { expect(issue).to be_closed }
it 'sends email to user2 about assign of new issue' do
email = ActionMailer::Base.deliveries.last
......@@ -32,7 +32,7 @@ describe Issues::CloseService, services: true do
end
it 'creates system note about issue reassign' do
note = @issue.notes.last
note = issue.notes.last
expect(note.note).to include "Status changed to closed"
end
......@@ -44,23 +44,43 @@ describe Issues::CloseService, services: true do
context 'current user is not authorized to close issue' do
before do
perform_enqueued_jobs do
@issue = described_class.new(project, guest).execute(issue)
described_class.new(project, guest).execute(issue)
end
end
it 'does not close the issue' do
expect(@issue).to be_open
expect(issue).to be_open
end
end
context "external issue tracker" do
context 'when issue is not confidential' do
it 'executes issue hooks' do
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
described_class.new(project, user).execute(issue)
end
end
context 'when issue is confidential' do
it 'executes confidential issue hooks' do
issue = create(:issue, :confidential, project: project)
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
described_class.new(project, user).execute(issue)
end
end
context 'external issue tracker' do
before do
allow(project).to receive(:default_issues_tracker?).and_return(false)
@issue = described_class.new(project, user, {}).execute(issue)
described_class.new(project, user).execute(issue)
end
it { expect(@issue).to be_valid }
it { expect(@issue).to be_opened }
it { expect(issue).to be_valid }
it { expect(issue).to be_opened }
it { expect(todo.reload).to be_pending }
end
end
......
......@@ -72,6 +72,24 @@ describe Issues::CreateService, services: true do
expect(issue.milestone).not_to eq milestone
end
end
it 'executes issue hooks when issue is not confidential' do
opts = { title: 'Title', description: 'Description', confidential: false }
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
described_class.new(project, user, opts).execute
end
it 'executes confidential issue hooks when issue is confidential' do
opts = { title: 'Title', description: 'Description', confidential: true }
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
described_class.new(project, user, opts).execute
end
end
it_behaves_like 'new issuable record that supports slash commands'
......
require 'spec_helper'
describe Issues::ReopenService, services: true do
let(:guest) { create(:user) }
let(:issue) { create(:issue, :closed) }
let(:project) { issue.project }
let(:project) { create(:empty_project) }
let(:issue) { create(:issue, :closed, project: project) }
describe '#execute' do
context 'when user is not authorized to reopen issue' do
before do
guest = create(:user)
project.team << [guest, :guest]
end
describe '#execute' do
context 'current user is not authorized to reopen issue' do
before do
perform_enqueued_jobs do
@issue = described_class.new(project, guest).execute(issue)
described_class.new(project, guest).execute(issue)
end
end
it 'does not reopen the issue' do
expect(@issue).to be_closed
expect(issue).to be_closed
end
end
context 'when user is authrized to reopen issue' do
let(:user) { create(:user) }
before do
project.team << [user, :master]
end
context 'when issue is not confidential' do
it 'executes issue hooks' do
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
described_class.new(project, user).execute(issue)
end
end
context 'when issue is confidential' do
it 'executes confidential issue hooks' do
issue = create(:issue, :confidential, :closed, project: project)
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
described_class.new(project, user).execute(issue)
end
end
end
end
......
......@@ -23,11 +23,15 @@ describe Issues::UpdateService, services: true do
describe 'execute' do
def find_note(starting_with)
@issue.notes.find do |note|
issue.notes.find do |note|
note && note.note.start_with?(starting_with)
end
end
def update_issue(opts)
described_class.new(project, user, opts).execute(issue)
end
context "valid params" do
before do
opts = {
......@@ -35,23 +39,20 @@ describe Issues::UpdateService, services: true do
description: 'Also please fix',
assignee_id: user2.id,
state_event: 'close',
label_ids: [label.id],
confidential: true
label_ids: [label.id]
}
perform_enqueued_jobs do
@issue = Issues::UpdateService.new(project, user, opts).execute(issue)
update_issue(opts)
end
@issue.reload
end
it { expect(@issue).to be_valid }
it { expect(@issue.title).to eq('New title') }
it { expect(@issue.assignee).to eq(user2) }
it { expect(@issue).to be_closed }
it { expect(@issue.labels.count).to eq(1) }
it { expect(@issue.labels.first.title).to eq(label.name) }
it { expect(issue).to be_valid }
it { expect(issue.title).to eq('New title') }
it { expect(issue.assignee).to eq(user2) }
it { expect(issue).to be_closed }
it { expect(issue.labels.count).to eq(1) }
it { expect(issue.labels.first.title).to eq(label.name) }
it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do
deliveries = ActionMailer::Base.deliveries
......@@ -81,18 +82,35 @@ describe Issues::UpdateService, services: true do
expect(note).not_to be_nil
expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
end
end
context 'when issue turns confidential' do
let(:opts) do
{
title: 'New title',
description: 'Also please fix',
assignee_id: user2.id,
state_event: 'close',
label_ids: [label.id],
confidential: true
}
end
it 'creates system note about confidentiality change' do
update_issue(confidential: true)
note = find_note('Made the issue confidential')
expect(note).not_to be_nil
expect(note.note).to eq 'Made the issue confidential'
end
end
def update_issue(opts)
@issue = Issues::UpdateService.new(project, user, opts).execute(issue)
@issue.reload
it 'executes confidential issue hooks' do
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
update_issue(confidential: true)
end
end
context 'todos' do
......@@ -100,7 +118,7 @@ describe Issues::UpdateService, services: true do
context 'when the title change' do
before do
update_issue({ title: 'New title' })
update_issue(title: 'New title')
end
it 'marks pending todos as done' do
......@@ -110,7 +128,7 @@ describe Issues::UpdateService, services: true do
context 'when the description change' do
before do
update_issue({ description: 'Also please fix' })
update_issue(description: 'Also please fix')
end
it 'marks todos as done' do
......@@ -120,7 +138,7 @@ describe Issues::UpdateService, services: true do
context 'when is reassigned' do
before do
update_issue({ assignee: user2 })
update_issue(assignee: user2)
end
it 'marks previous assignee todos as done' do
......@@ -144,7 +162,7 @@ describe Issues::UpdateService, services: true do
context 'when the milestone change' do
before do
update_issue({ milestone: create(:milestone) })
update_issue(milestone: create(:milestone))
end
it 'marks todos as done' do
......@@ -154,7 +172,7 @@ describe Issues::UpdateService, services: true do
context 'when the labels change' do
before do
update_issue({ label_ids: [label.id] })
update_issue(label_ids: [label.id])
end
it 'marks todos as done' do
......@@ -165,6 +183,7 @@ describe Issues::UpdateService, services: true do
context 'when the issue is relabeled' do
let!(:non_subscriber) { create(:user) }
let!(:subscriber) do
create(:user).tap do |u|
label.toggle_subscription(u)
......@@ -176,7 +195,7 @@ describe Issues::UpdateService, services: true do
opts = { label_ids: [label.id] }
perform_enqueued_jobs do
@issue = Issues::UpdateService.new(project, user, opts).execute(issue)
@issue = described_class.new(project, user, opts).execute(issue)
end
should_email(subscriber)
......@@ -190,7 +209,7 @@ describe Issues::UpdateService, services: true do
opts = { label_ids: [label.id, label2.id] }
perform_enqueued_jobs do
@issue = Issues::UpdateService.new(project, user, opts).execute(issue)
@issue = described_class.new(project, user, opts).execute(issue)
end
should_not_email(subscriber)
......@@ -201,7 +220,7 @@ describe Issues::UpdateService, services: true do
opts = { label_ids: [label2.id] }
perform_enqueued_jobs do
@issue = Issues::UpdateService.new(project, user, opts).execute(issue)
@issue = described_class.new(project, user, opts).execute(issue)
end
should_not_email(subscriber)
......@@ -210,13 +229,15 @@ describe Issues::UpdateService, services: true do
end
end
context 'when Issue has tasks' do
before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
context 'when issue has tasks' do
before do
update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
end
it { expect(@issue.tasks?).to eq(true) }
it { expect(issue.tasks?).to eq(true) }
context 'when tasks are marked as completed' do
before { update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) }
before { update_issue(description: "- [x] Task 1\n- [X] Task 2") }
it 'creates system note about task status change' do
note1 = find_note('Marked the task **Task 1** as completed')
......@@ -229,8 +250,8 @@ describe Issues::UpdateService, services: true do
context 'when tasks are marked as incomplete' do
before do
update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" })
update_issue(description: "- [x] Task 1\n- [X] Task 2")
update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
end
it 'creates system note about task status change' do
......@@ -244,8 +265,8 @@ describe Issues::UpdateService, services: true do
context 'when tasks position has been modified' do
before do
update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
update_issue({ description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2" })
update_issue(description: "- [x] Task 1\n- [X] Task 2")
update_issue(description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2")
end
it 'does not create a system note' do
......@@ -257,8 +278,8 @@ describe Issues::UpdateService, services: true do
context 'when a Task list with a completed item is totally replaced' do
before do
update_issue({ description: "- [ ] Task 1\n- [X] Task 2" })
update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
update_issue(description: "- [ ] Task 1\n- [X] Task 2")
update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three")
end
it 'does not create a system note referencing the position the old item' do
......@@ -269,7 +290,7 @@ describe Issues::UpdateService, services: true do
it 'does not generate a new note at all' do
expect do
update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three")
end.not_to change { Note.count }
end
end
......@@ -277,7 +298,7 @@ describe Issues::UpdateService, services: true do
context 'updating labels' do
let(:label3) { create(:label, project: project) }
let(:result) { Issues::UpdateService.new(project, user, params).execute(issue).reload }
let(:result) { described_class.new(project, user, params).execute(issue).reload }
context 'when add_label_ids and label_ids are passed' do
let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } }
......
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