diff --git a/Gemfile b/Gemfile
index 1c822a40fc6cda96e78159ead695a00335ae4054..6d820e1f88782ab77e2794b202fb69838b34374f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -82,7 +82,7 @@ gem 'net-ldap'
# Git Wiki
# Required manually in config/initializers/gollum.rb to control load order
-gem 'gitlab-gollum-lib', '~> 4.2'
+gem 'gitlab-gollum-lib', '~> 4.2', require: false
gem 'gitlab-gollum-rugged_adapter', '~> 0.4.4', require: false
@@ -415,7 +415,7 @@ group :ed25519 do
end
# Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.97.0', require: 'gitaly'
gem 'grpc', '~> 1.10.0'
# Locked until https://github.com/google/protobuf/issues/4210 is closed
diff --git a/Gemfile.lock b/Gemfile.lock
index 7f243491c90eb5e231ba592cf8b83ef0af2cf5a5..3076fbee6a88dea99a92dc0d5be5eec6342edbdd 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -290,9 +290,9 @@ GEM
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gherkin-ruby (0.3.2)
- gitaly-proto (0.94.0)
+ gitaly-proto (0.97.0)
google-protobuf (~> 3.1)
- grpc (~> 1.0)
+ grpc (~> 1.10)
github-linguist (5.3.3)
charlock_holmes (~> 0.7.5)
escape_utils (~> 1.1.0)
@@ -1064,7 +1064,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly-proto (~> 0.94.0)
+ gitaly-proto (~> 0.97.0)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2)
diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb
index c4930d3d18d736d36504bdaf8a0602ddc1662b4e..1b0751f48c57006456e41f1e06ad0dcfe3b83490 100644
--- a/app/controllers/projects/wikis_controller.rb
+++ b/app/controllers/projects/wikis_controller.rb
@@ -29,8 +29,7 @@ def show
else
return render('empty') unless can?(current_user, :create_wiki, @project)
- @page = WikiPage.new(@project_wiki)
- @page.title = params[:id]
+ @page = build_page(title: params[:id])
render 'edit'
end
@@ -54,7 +53,7 @@ def update
else
render 'edit'
end
- rescue WikiPage::PageChangedError, WikiPage::PageRenameError => e
+ rescue WikiPage::PageChangedError, WikiPage::PageRenameError, Gitlab::Git::Wiki::OperationError => e
@error = e
render 'edit'
end
@@ -70,6 +69,11 @@ def create
else
render action: "edit"
end
+ rescue Gitlab::Git::Wiki::OperationError => e
+ @page = build_page(wiki_params)
+ @error = e
+
+ render 'edit'
end
def history
@@ -94,6 +98,9 @@ def destroy
redirect_to project_wiki_path(@project, :home),
status: 302,
notice: "Page was successfully deleted"
+ rescue Gitlab::Git::Wiki::OperationError => e
+ @error = e
+ render 'edit'
end
def git_access
@@ -116,4 +123,10 @@ def load_project_wiki
def wiki_params
params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha)
end
+
+ def build_page(args)
+ WikiPage.new(@project_wiki).tap do |page|
+ page.update_attributes(args)
+ end
+ end
end
diff --git a/app/helpers/nav_helper.rb b/app/helpers/nav_helper.rb
index 56c88e6eab0db7b9a92651faf22cb0a1fe62fdb3..7754c34d6f03c6c8d4931a679d29d7f7332a157d 100644
--- a/app/helpers/nav_helper.rb
+++ b/app/helpers/nav_helper.rb
@@ -28,7 +28,7 @@ def page_gutter_class
end
elsif current_path?('jobs#show')
%w[page-gutter build-sidebar right-sidebar-expanded]
- elsif current_controller?('wikis') && current_action?('show', 'create', 'edit', 'update', 'history', 'git_access')
+ elsif current_controller?('wikis') && current_action?('show', 'create', 'edit', 'update', 'history', 'git_access', 'destroy')
%w[page-gutter wiki-sidebar right-sidebar-expanded]
else
[]
diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb
index 52e067cb44c5ffddcaa591792d9e739bcb83de71..b7e38ceb5021a0dfdab7269914b528b9396166af 100644
--- a/app/models/project_wiki.rb
+++ b/app/models/project_wiki.rb
@@ -179,7 +179,11 @@ def create_repo!(raw_repository)
def commit_details(action, message = nil, title = nil)
commit_message = message || default_message(action, title)
- Gitlab::Git::Wiki::CommitDetails.new(@user.name, @user.email, commit_message)
+ Gitlab::Git::Wiki::CommitDetails.new(@user.id,
+ @user.username,
+ @user.name,
+ @user.email,
+ commit_message)
end
def default_message(action, title)
diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb
index 0f5536415f79ceea274c468cc0da5f14694d2799..cde79b95062373e177ccd37503267477bb1e9930 100644
--- a/app/models/wiki_page.rb
+++ b/app/models/wiki_page.rb
@@ -265,6 +265,15 @@ def title_changed?
title.present? && self.class.unhyphenize(@page.url_path) != title
end
+ # Updates the current @attributes hash by merging a hash of params
+ def update_attributes(attrs)
+ attrs[:title] = process_title(attrs[:title]) if attrs[:title].present?
+
+ attrs.slice!(:content, :format, :message, :title)
+
+ @attributes.merge!(attrs)
+ end
+
private
# Process and format the title based on the user input.
@@ -290,15 +299,6 @@ def deep_title_squish(title)
File.join(components)
end
- # Updates the current @attributes hash by merging a hash of params
- def update_attributes(attrs)
- attrs[:title] = process_title(attrs[:title]) if attrs[:title].present?
-
- attrs.slice!(:content, :format, :message, :title)
-
- @attributes.merge!(attrs)
- end
-
def set_attributes
attributes[:slug] = @page.url_path
attributes[:title] = @page.title
diff --git a/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml b/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml
new file mode 100644
index 0000000000000000000000000000000000000000..9fe458aba4a4937bab91efce7d1027cc36f2d67a
--- /dev/null
+++ b/changelogs/unreleased/fj-42354-custom-hooks-not-triggered-by-UI-wiki-edit.yml
@@ -0,0 +1,5 @@
+---
+title: Triggering custom hooks by Wiki UI edit
+merge_request: 18251
+author:
+type: fixed
diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb
index 8d82820915db63adbab1f7e957adba6f13ffdbc8..821436911ab50bd2abe9b7ff3f604c0d2ae712cf 100644
--- a/lib/gitlab/git/wiki.rb
+++ b/lib/gitlab/git/wiki.rb
@@ -2,10 +2,11 @@ module Gitlab
module Git
class Wiki
DuplicatePageError = Class.new(StandardError)
+ OperationError = Class.new(StandardError)
- CommitDetails = Struct.new(:name, :email, :message) do
+ CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do
def to_h
- { name: name, email: email, message: message }
+ { user_id: user_id, username: username, name: name, email: email, message: message }
end
end
PageBlob = Struct.new(:name)
@@ -140,6 +141,10 @@ def page_formatted_data(title:, dir: nil, version: nil)
end
end
+ def gollum_wiki
+ @gollum_wiki ||= Gollum::Wiki.new(@repository.path)
+ end
+
private
# options:
@@ -158,10 +163,6 @@ def commits_from_page(gollum_page, options = {})
offset: options[:offset])
end
- def gollum_wiki
- @gollum_wiki ||= Gollum::Wiki.new(@repository.path)
- end
-
def gollum_page_by_path(page_path)
page_name = Gollum::Page.canonicalize_filename(page_path)
page_dir = File.split(page_path).first
@@ -201,12 +202,12 @@ def gollum_write_page(name, format, content, commit_details)
assert_type!(format, Symbol)
assert_type!(commit_details, CommitDetails)
- filename = File.basename(name)
- dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir
-
- gollum_wiki.write_page(filename, format, content, commit_details.to_h, dir)
+ with_committer_with_hooks(commit_details) do |committer|
+ filename = File.basename(name)
+ dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir
- nil
+ gollum_wiki.write_page(filename, format, content, { committer: committer }, dir)
+ end
rescue Gollum::DuplicatePageError => e
raise Gitlab::Git::Wiki::DuplicatePageError, e.message
end
@@ -214,24 +215,23 @@ def gollum_write_page(name, format, content, commit_details)
def gollum_delete_page(page_path, commit_details)
assert_type!(commit_details, CommitDetails)
- gollum_wiki.delete_page(gollum_page_by_path(page_path), commit_details.to_h)
- nil
+ with_committer_with_hooks(commit_details) do |committer|
+ gollum_wiki.delete_page(gollum_page_by_path(page_path), committer: committer)
+ end
end
def gollum_update_page(page_path, title, format, content, commit_details)
assert_type!(format, Symbol)
assert_type!(commit_details, CommitDetails)
- page = gollum_page_by_path(page_path)
- committer = Gollum::Committer.new(page.wiki, commit_details.to_h)
-
- # Instead of performing two renames if the title has changed,
- # the update_page will only update the format and content and
- # the rename_page will do anything related to moving/renaming
- gollum_wiki.update_page(page, page.name, format, content, committer: committer)
- gollum_wiki.rename_page(page, title, committer: committer)
- committer.commit
- nil
+ with_committer_with_hooks(commit_details) do |committer|
+ page = gollum_page_by_path(page_path)
+ # Instead of performing two renames if the title has changed,
+ # the update_page will only update the format and content and
+ # the rename_page will do anything related to moving/renaming
+ gollum_wiki.update_page(page, page.name, format, content, committer: committer)
+ gollum_wiki.rename_page(page, title, committer: committer)
+ end
end
def gollum_find_page(title:, version: nil, dir: nil)
@@ -288,6 +288,20 @@ def gitaly_get_all_pages
Gitlab::Git::WikiPage.new(wiki_page, version)
end
end
+
+ def committer_with_hooks(commit_details)
+ Gitlab::Wiki::CommitterWithHooks.new(self, commit_details.to_h)
+ end
+
+ def with_committer_with_hooks(commit_details, &block)
+ committer = committer_with_hooks(commit_details)
+
+ yield committer
+
+ committer.commit
+
+ nil
+ end
end
end
end
diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb
index 7a698e4b3f39e58609128c438dc5f468ba157b74..2dfe055a496b9548ee851b0c2d32e2c13959ba81 100644
--- a/lib/gitlab/gitaly_client/wiki_service.rb
+++ b/lib/gitlab/gitaly_client/wiki_service.rb
@@ -200,6 +200,8 @@ def new_wiki_page_version(version)
def gitaly_commit_details(commit_details)
Gitaly::WikiCommitDetails.new(
+ user_id: commit_details.user_id,
+ user_name: encode_binary(commit_details.username),
name: encode_binary(commit_details.name),
email: encode_binary(commit_details.email),
message: encode_binary(commit_details.message)
diff --git a/lib/gitlab/gl_id.rb b/lib/gitlab/gl_id.rb
index 624fd00367e0f5c128497449630242824b62e416..a53d156b41f3fa7e28204d5b301c4fed62329153 100644
--- a/lib/gitlab/gl_id.rb
+++ b/lib/gitlab/gl_id.rb
@@ -2,10 +2,14 @@ module Gitlab
module GlId
def self.gl_id(user)
if user.present?
- "user-#{user.id}"
+ gl_id_from_id_value(user.id)
else
- ""
+ ''
end
end
+
+ def self.gl_id_from_id_value(id)
+ "user-#{id}"
+ end
end
end
diff --git a/lib/gitlab/wiki/committer_with_hooks.rb b/lib/gitlab/wiki/committer_with_hooks.rb
new file mode 100644
index 0000000000000000000000000000000000000000..19f0b3814fd48e998043274d197a33faef2edaf0
--- /dev/null
+++ b/lib/gitlab/wiki/committer_with_hooks.rb
@@ -0,0 +1,39 @@
+module Gitlab
+ module Wiki
+ class CommitterWithHooks < Gollum::Committer
+ attr_reader :gl_wiki
+
+ def initialize(gl_wiki, options = {})
+ @gl_wiki = gl_wiki
+ super(gl_wiki.gollum_wiki, options)
+ end
+
+ def commit
+ result = Gitlab::Git::OperationService.new(git_user, gl_wiki.repository).with_branch(
+ @wiki.ref,
+ start_branch_name: @wiki.ref
+ ) do |start_commit|
+ super(false)
+ end
+
+ result[:newrev]
+ rescue Gitlab::Git::HooksService::PreReceiveError => e
+ message = "Custom Hook failed: #{e.message}"
+ raise Gitlab::Git::Wiki::OperationError, message
+ end
+
+ private
+
+ def git_user
+ @git_user ||= Gitlab::Git::User.new(@options[:username],
+ @options[:name],
+ @options[:email],
+ gitlab_id)
+ end
+
+ def gitlab_id
+ Gitlab::GlId.gl_id_from_id_value(@options[:user_id])
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/git/wiki_spec.rb b/spec/lib/gitlab/git/wiki_spec.rb
index 761f77320365b18bdd3f0b6986d3dbb4c7a5dc37..722d697c28e8cbdaaa43a52a47af5098dab1c65e 100644
--- a/spec/lib/gitlab/git/wiki_spec.rb
+++ b/spec/lib/gitlab/git/wiki_spec.rb
@@ -30,7 +30,7 @@ def create_page(name, content)
end
def commit_details(name)
- Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "created page #{name}")
+ Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "created page #{name}")
end
def destroy_page(title, dir = '')
diff --git a/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb b/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..830fb8a85983f64287942ce5d211876edaac230b
--- /dev/null
+++ b/spec/lib/gitlab/wiki/committer_with_hooks_spec.rb
@@ -0,0 +1,154 @@
+require 'spec_helper'
+
+describe Gitlab::Wiki::CommitterWithHooks, seed_helper: true do
+ shared_examples 'calling wiki hooks' do
+ let(:project) { create(:project) }
+ let(:user) { project.owner }
+ let(:project_wiki) { ProjectWiki.new(project, user) }
+ let(:wiki) { project_wiki.wiki }
+ let(:options) do
+ {
+ id: user.id,
+ username: user.username,
+ name: user.name,
+ email: user.email,
+ message: 'commit message'
+ }
+ end
+
+ subject { described_class.new(wiki, options) }
+
+ before do
+ project_wiki.create_page('home', 'test content')
+ end
+
+ shared_examples 'failing pre-receive hook' do
+ before do
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([false, ''])
+ expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('update')
+ expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('post-receive')
+ end
+
+ it 'raises exception' do
+ expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError)
+ end
+
+ it 'does not create a new commit inside the repository' do
+ current_rev = find_current_rev
+
+ expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError)
+
+ expect(current_rev).to eq find_current_rev
+ end
+ end
+
+ shared_examples 'failing update hook' do
+ before do
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([true, ''])
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('update').and_return([false, ''])
+ expect_any_instance_of(Gitlab::Git::HooksService).not_to receive(:run_hook).with('post-receive')
+ end
+
+ it 'raises exception' do
+ expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError)
+ end
+
+ it 'does not create a new commit inside the repository' do
+ current_rev = find_current_rev
+
+ expect { subject.commit }.to raise_error(Gitlab::Git::Wiki::OperationError)
+
+ expect(current_rev).to eq find_current_rev
+ end
+ end
+
+ shared_examples 'failing post-receive hook' do
+ before do
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('pre-receive').and_return([true, ''])
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('update').and_return([true, ''])
+ expect_any_instance_of(Gitlab::Git::HooksService).to receive(:run_hook).with('post-receive').and_return([false, ''])
+ end
+
+ it 'does not raise exception' do
+ expect { subject.commit }.not_to raise_error
+ end
+
+ it 'creates the commit' do
+ current_rev = find_current_rev
+
+ subject.commit
+
+ expect(current_rev).not_to eq find_current_rev
+ end
+ end
+
+ shared_examples 'when hooks call succceeds' do
+ let(:hook) { double(:hook) }
+
+ it 'calls the three hooks' do
+ expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
+ expect(hook).to receive(:trigger).exactly(3).times.and_return([true, nil])
+
+ subject.commit
+ end
+
+ it 'creates the commit' do
+ current_rev = find_current_rev
+
+ subject.commit
+
+ expect(current_rev).not_to eq find_current_rev
+ end
+ end
+
+ context 'when creating a page' do
+ before do
+ project_wiki.create_page('index', 'test content')
+ end
+
+ it_behaves_like 'failing pre-receive hook'
+ it_behaves_like 'failing update hook'
+ it_behaves_like 'failing post-receive hook'
+ it_behaves_like 'when hooks call succceeds'
+ end
+
+ context 'when updating a page' do
+ before do
+ project_wiki.update_page(find_page('home'), content: 'some other content', format: :markdown)
+ end
+
+ it_behaves_like 'failing pre-receive hook'
+ it_behaves_like 'failing update hook'
+ it_behaves_like 'failing post-receive hook'
+ it_behaves_like 'when hooks call succceeds'
+ end
+
+ context 'when deleting a page' do
+ before do
+ project_wiki.delete_page(find_page('home'))
+ end
+
+ it_behaves_like 'failing pre-receive hook'
+ it_behaves_like 'failing update hook'
+ it_behaves_like 'failing post-receive hook'
+ it_behaves_like 'when hooks call succceeds'
+ end
+
+ def find_current_rev
+ wiki.gollum_wiki.repo.commits.first&.sha
+ end
+
+ def find_page(name)
+ wiki.page(title: name)
+ end
+ end
+
+ # TODO: Uncomment once Gitaly updates the ruby vendor code
+ # context 'when Gitaly is enabled' do
+ # it_behaves_like 'calling wiki hooks'
+ # end
+
+ context 'when Gitaly is disabled', :skip_gitaly_mock do
+ it_behaves_like 'calling wiki hooks'
+ end
+end
diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb
index 374a157bec007147d6a61a2a1b4cb9c09e8b2e8a..4e83f4353cf8540d29c8518f89b4b9c85b8438bb 100644
--- a/spec/models/project_wiki_spec.rb
+++ b/spec/models/project_wiki_spec.rb
@@ -377,7 +377,7 @@ def remove_temp_repo(path)
end
def commit_details
- Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit")
+ Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit")
end
def create_page(name, content)
diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb
index b2b7721674c5797efcf92018fd30912ed058385d..90b7e7715a84da0a51051d274dc1fc870225b80b 100644
--- a/spec/models/wiki_page_spec.rb
+++ b/spec/models/wiki_page_spec.rb
@@ -561,7 +561,7 @@ def remove_temp_repo(path)
end
def commit_details
- Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit")
+ Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit")
end
def create_page(name, content)