Commit e8a27a67 authored by Francisco Javier López's avatar Francisco Javier López Committed by Douwe Maan

Fix Custom hooks are not triggered by UI wiki edit

parent 6a5de6dd
...@@ -82,7 +82,7 @@ gem 'net-ldap' ...@@ -82,7 +82,7 @@ gem 'net-ldap'
# Git Wiki # Git Wiki
# Required manually in config/initializers/gollum.rb to control load order # 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 gem 'gitlab-gollum-rugged_adapter', '~> 0.4.4', require: false
...@@ -415,7 +415,7 @@ group :ed25519 do ...@@ -415,7 +415,7 @@ group :ed25519 do
end end
# Gitaly GRPC client # Gitaly GRPC client
gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly' gem 'gitaly-proto', '~> 0.97.0', require: 'gitaly'
gem 'grpc', '~> 1.10.0' gem 'grpc', '~> 1.10.0'
# Locked until https://github.com/google/protobuf/issues/4210 is closed # Locked until https://github.com/google/protobuf/issues/4210 is closed
......
...@@ -290,9 +290,9 @@ GEM ...@@ -290,9 +290,9 @@ GEM
po_to_json (>= 1.0.0) po_to_json (>= 1.0.0)
rails (>= 3.2.0) rails (>= 3.2.0)
gherkin-ruby (0.3.2) gherkin-ruby (0.3.2)
gitaly-proto (0.94.0) gitaly-proto (0.97.0)
google-protobuf (~> 3.1) google-protobuf (~> 3.1)
grpc (~> 1.0) grpc (~> 1.10)
github-linguist (5.3.3) github-linguist (5.3.3)
charlock_holmes (~> 0.7.5) charlock_holmes (~> 0.7.5)
escape_utils (~> 1.1.0) escape_utils (~> 1.1.0)
...@@ -1064,7 +1064,7 @@ DEPENDENCIES ...@@ -1064,7 +1064,7 @@ DEPENDENCIES
gettext (~> 3.2.2) gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3) gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.94.0) gitaly-proto (~> 0.97.0)
github-linguist (~> 5.3.3) github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1) gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2) gitlab-gollum-lib (~> 4.2)
......
...@@ -29,8 +29,7 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -29,8 +29,7 @@ class Projects::WikisController < Projects::ApplicationController
else else
return render('empty') unless can?(current_user, :create_wiki, @project) return render('empty') unless can?(current_user, :create_wiki, @project)
@page = WikiPage.new(@project_wiki) @page = build_page(title: params[:id])
@page.title = params[:id]
render 'edit' render 'edit'
end end
...@@ -54,7 +53,7 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -54,7 +53,7 @@ class Projects::WikisController < Projects::ApplicationController
else else
render 'edit' render 'edit'
end end
rescue WikiPage::PageChangedError, WikiPage::PageRenameError => e rescue WikiPage::PageChangedError, WikiPage::PageRenameError, Gitlab::Git::Wiki::OperationError => e
@error = e @error = e
render 'edit' render 'edit'
end end
...@@ -70,6 +69,11 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -70,6 +69,11 @@ class Projects::WikisController < Projects::ApplicationController
else else
render action: "edit" render action: "edit"
end end
rescue Gitlab::Git::Wiki::OperationError => e
@page = build_page(wiki_params)
@error = e
render 'edit'
end end
def history def history
...@@ -94,6 +98,9 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -94,6 +98,9 @@ class Projects::WikisController < Projects::ApplicationController
redirect_to project_wiki_path(@project, :home), redirect_to project_wiki_path(@project, :home),
status: 302, status: 302,
notice: "Page was successfully deleted" notice: "Page was successfully deleted"
rescue Gitlab::Git::Wiki::OperationError => e
@error = e
render 'edit'
end end
def git_access def git_access
...@@ -116,4 +123,10 @@ class Projects::WikisController < Projects::ApplicationController ...@@ -116,4 +123,10 @@ class Projects::WikisController < Projects::ApplicationController
def wiki_params def wiki_params
params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha) params.require(:wiki).permit(:title, :content, :format, :message, :last_commit_sha)
end end
def build_page(args)
WikiPage.new(@project_wiki).tap do |page|
page.update_attributes(args)
end
end
end end
...@@ -28,7 +28,7 @@ module NavHelper ...@@ -28,7 +28,7 @@ module NavHelper
end end
elsif current_path?('jobs#show') elsif current_path?('jobs#show')
%w[page-gutter build-sidebar right-sidebar-expanded] %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] %w[page-gutter wiki-sidebar right-sidebar-expanded]
else else
[] []
......
...@@ -179,7 +179,11 @@ class ProjectWiki ...@@ -179,7 +179,11 @@ class ProjectWiki
def commit_details(action, message = nil, title = nil) def commit_details(action, message = nil, title = nil)
commit_message = message || default_message(action, title) 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 end
def default_message(action, title) def default_message(action, title)
......
...@@ -265,6 +265,15 @@ class WikiPage ...@@ -265,6 +265,15 @@ class WikiPage
title.present? && self.class.unhyphenize(@page.url_path) != title title.present? && self.class.unhyphenize(@page.url_path) != title
end 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 private
# Process and format the title based on the user input. # Process and format the title based on the user input.
...@@ -290,15 +299,6 @@ class WikiPage ...@@ -290,15 +299,6 @@ class WikiPage
File.join(components) File.join(components)
end 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 def set_attributes
attributes[:slug] = @page.url_path attributes[:slug] = @page.url_path
attributes[:title] = @page.title attributes[:title] = @page.title
......
---
title: Triggering custom hooks by Wiki UI edit
merge_request: 18251
author:
type: fixed
...@@ -2,10 +2,11 @@ module Gitlab ...@@ -2,10 +2,11 @@ module Gitlab
module Git module Git
class Wiki class Wiki
DuplicatePageError = Class.new(StandardError) 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 def to_h
{ name: name, email: email, message: message } { user_id: user_id, username: username, name: name, email: email, message: message }
end end
end end
PageBlob = Struct.new(:name) PageBlob = Struct.new(:name)
...@@ -140,6 +141,10 @@ module Gitlab ...@@ -140,6 +141,10 @@ module Gitlab
end end
end end
def gollum_wiki
@gollum_wiki ||= Gollum::Wiki.new(@repository.path)
end
private private
# options: # options:
...@@ -158,10 +163,6 @@ module Gitlab ...@@ -158,10 +163,6 @@ module Gitlab
offset: options[:offset]) offset: options[:offset])
end end
def gollum_wiki
@gollum_wiki ||= Gollum::Wiki.new(@repository.path)
end
def gollum_page_by_path(page_path) def gollum_page_by_path(page_path)
page_name = Gollum::Page.canonicalize_filename(page_path) page_name = Gollum::Page.canonicalize_filename(page_path)
page_dir = File.split(page_path).first page_dir = File.split(page_path).first
...@@ -201,12 +202,12 @@ module Gitlab ...@@ -201,12 +202,12 @@ module Gitlab
assert_type!(format, Symbol) assert_type!(format, Symbol)
assert_type!(commit_details, CommitDetails) assert_type!(commit_details, CommitDetails)
filename = File.basename(name) with_committer_with_hooks(commit_details) do |committer|
dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir filename = File.basename(name)
dir = (tmp_dir = File.dirname(name)) == '.' ? '' : tmp_dir
gollum_wiki.write_page(filename, format, content, commit_details.to_h, dir)
nil gollum_wiki.write_page(filename, format, content, { committer: committer }, dir)
end
rescue Gollum::DuplicatePageError => e rescue Gollum::DuplicatePageError => e
raise Gitlab::Git::Wiki::DuplicatePageError, e.message raise Gitlab::Git::Wiki::DuplicatePageError, e.message
end end
...@@ -214,24 +215,23 @@ module Gitlab ...@@ -214,24 +215,23 @@ module Gitlab
def gollum_delete_page(page_path, commit_details) def gollum_delete_page(page_path, commit_details)
assert_type!(commit_details, CommitDetails) assert_type!(commit_details, CommitDetails)
gollum_wiki.delete_page(gollum_page_by_path(page_path), commit_details.to_h) with_committer_with_hooks(commit_details) do |committer|
nil gollum_wiki.delete_page(gollum_page_by_path(page_path), committer: committer)
end
end end
def gollum_update_page(page_path, title, format, content, commit_details) def gollum_update_page(page_path, title, format, content, commit_details)
assert_type!(format, Symbol) assert_type!(format, Symbol)
assert_type!(commit_details, CommitDetails) assert_type!(commit_details, CommitDetails)
page = gollum_page_by_path(page_path) with_committer_with_hooks(commit_details) do |committer|
committer = Gollum::Committer.new(page.wiki, commit_details.to_h) page = gollum_page_by_path(page_path)
# Instead of performing two renames if the title has changed,
# Instead of performing two renames if the title has changed, # the update_page will only update the format and content and
# the update_page will only update the format and content and # the rename_page will do anything related to moving/renaming
# the rename_page will do anything related to moving/renaming gollum_wiki.update_page(page, page.name, format, content, committer: committer)
gollum_wiki.update_page(page, page.name, format, content, committer: committer) gollum_wiki.rename_page(page, title, committer: committer)
gollum_wiki.rename_page(page, title, committer: committer) end
committer.commit
nil
end end
def gollum_find_page(title:, version: nil, dir: nil) def gollum_find_page(title:, version: nil, dir: nil)
...@@ -288,6 +288,20 @@ module Gitlab ...@@ -288,6 +288,20 @@ module Gitlab
Gitlab::Git::WikiPage.new(wiki_page, version) Gitlab::Git::WikiPage.new(wiki_page, version)
end end
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 end
end end
...@@ -200,6 +200,8 @@ module Gitlab ...@@ -200,6 +200,8 @@ module Gitlab
def gitaly_commit_details(commit_details) def gitaly_commit_details(commit_details)
Gitaly::WikiCommitDetails.new( Gitaly::WikiCommitDetails.new(
user_id: commit_details.user_id,
user_name: encode_binary(commit_details.username),
name: encode_binary(commit_details.name), name: encode_binary(commit_details.name),
email: encode_binary(commit_details.email), email: encode_binary(commit_details.email),
message: encode_binary(commit_details.message) message: encode_binary(commit_details.message)
......
...@@ -2,10 +2,14 @@ module Gitlab ...@@ -2,10 +2,14 @@ module Gitlab
module GlId module GlId
def self.gl_id(user) def self.gl_id(user)
if user.present? if user.present?
"user-#{user.id}" gl_id_from_id_value(user.id)
else else
"" ''
end end
end end
def self.gl_id_from_id_value(id)
"user-#{id}"
end
end end
end end
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
...@@ -30,7 +30,7 @@ describe Gitlab::Git::Wiki do ...@@ -30,7 +30,7 @@ describe Gitlab::Git::Wiki do
end end
def commit_details(name) 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 end
def destroy_page(title, dir = '') def destroy_page(title, dir = '')
......
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
...@@ -377,7 +377,7 @@ describe ProjectWiki do ...@@ -377,7 +377,7 @@ describe ProjectWiki do
end end
def commit_details 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 end
def create_page(name, content) def create_page(name, content)
......
...@@ -561,7 +561,7 @@ describe WikiPage do ...@@ -561,7 +561,7 @@ describe WikiPage do
end end
def commit_details 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 end
def create_page(name, content) def create_page(name, content)
......
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