diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-05-13 14:26:39 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-05-13 14:26:39 +0300 |
commit | 63403019d286ae5dc7ff2481f8a948da96057489 (patch) | |
tree | 711eafd76030433ef6ba6dfcb1887c1902b095a9 | |
parent | f32a045ef41688859732abfcdc2a2030387be78f (diff) | |
parent | 740716afd16e6487c965caf9c2f63e99fc78c076 (diff) |
Merge branch 'master' of github.com:gitlabhq/gitlabhq
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/helpers/gitlab_markdown_helper.rb | 140 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 15 | ||||
-rw-r--r-- | doc/web_hooks/web_hooks.md | 2 | ||||
-rw-r--r-- | lib/gitlab/markdown.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/markdown/relative_link_filter.rb | 114 | ||||
-rw-r--r-- | lib/redcarpet/render/gitlab_html.rb | 4 | ||||
-rw-r--r-- | spec/helpers/gitlab_markdown_helper_spec.rb | 73 | ||||
-rw-r--r-- | spec/lib/gitlab/markdown/relative_link_filter_spec.rb | 120 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 18 |
10 files changed, 275 insertions, 221 deletions
diff --git a/CHANGELOG b/CHANGELOG index ffe5169bfd2..f059345a5a3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -56,6 +56,7 @@ v 7.11.0 (unreleased) - Add "Create Merge Request" buttons to commits and branches pages and push event. - Show user roles by comments. - Fix automatic blocking of auto-created users from Active Directory. + - Call merge request web hook for each new commits (Arthur Gautier) v 7.10.2 - Fix CI links on MR page diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 24263a0f619..c309f890d96 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -72,146 +72,6 @@ module GitlabMarkdownHelper end end - # TODO (rspeicher): This should be its own filter - def create_relative_links(text) - paths = extract_paths(text) - - paths.uniq.each do |file_path| - # If project does not have repository - # its nothing to rebuild - # - # TODO: pass project variable to markdown helper instead of using - # instance variable. Right now it generates invalid path for pages out - # of project scope. Example: search results where can be rendered markdown - # from different projects - if @repository && @repository.exists? && !@repository.empty? - new_path = rebuild_path(file_path) - # Finds quoted path so we don't replace other mentions of the string - # eg. "doc/api" will be replaced and "/home/doc/api/text" won't - text.gsub!("\"#{file_path}\"", "\"/#{new_path}\"") - end - end - - text - end - - def extract_paths(text) - links = substitute_links(text) - image_links = substitute_image_links(text) - links + image_links - end - - def substitute_links(text) - links = text.scan(/<a href=\"([^"]*)\">/) - relative_links = links.flatten.reject{ |link| link_to_ignore? link } - relative_links - end - - def substitute_image_links(text) - links = text.scan(/<img src=\"([^"]*)\"/) - relative_links = links.flatten.reject{ |link| link_to_ignore? link } - relative_links - end - - def link_to_ignore?(link) - if link =~ /\A\#\w+/ - # ignore anchors like <a href="#my-header"> - true - else - ignored_protocols.map{ |protocol| link.include?(protocol) }.any? - end - end - - def ignored_protocols - ["http://","https://", "ftp://", "mailto:", "smb://"] - end - - def rebuild_path(file_path) - file_path = file_path.dup - file_path.gsub!(/(#.*)/, "") - id = $1 || "" - file_path = relative_file_path(file_path) - file_path = sanitize_slashes(file_path) - - [ - Gitlab.config.gitlab.relative_url_root, - @project.path_with_namespace, - path_with_ref(file_path), - file_path - ].compact.join("/").gsub(/\A\/*|\/*\z/, '') + id - end - - def sanitize_slashes(path) - path[0] = "" if path.start_with?("/") - path.chop if path.end_with?("/") - path - end - - def relative_file_path(path) - requested_path = @path - nested_path = build_nested_path(path, requested_path) - return nested_path if file_exists?(nested_path) - path - end - - # Covering a special case, when the link is referencing file in the same directory eg: - # If we are at doc/api/README.md and the README.md contains relative links like [Users](users.md) - # this takes the request path(doc/api/README.md), and replaces the README.md with users.md so the path looks like doc/api/users.md - # If we are at doc/api and the README.md shown in below the tree view - # this takes the request path(doc/api) and adds users.md so the path looks like doc/api/users.md - def build_nested_path(path, request_path) - return request_path if path == "" - return path unless request_path - if local_path(request_path) == "tree" - base = request_path.split("/").push(path) - base.join("/") - else - base = request_path.split("/") - base.pop - base.push(path).join("/") - end - end - - # Checks if the path exists in the repo - # eg. checks if doc/README.md exists, if not then link to blob - def path_with_ref(path) - if file_exists?(path) - "#{local_path(path)}/#{correct_ref}" - else - "blob/#{correct_ref}" - end - end - - def file_exists?(path) - return false if path.nil? - @repository.blob_at(current_sha, path).present? || @repository.tree(current_sha, path).entries.any? - end - - # Check if the path is pointing to a directory(tree) or a file(blob) - # eg. doc/api is directory and doc/README.md is file - def local_path(path) - return "tree" if @repository.tree(current_sha, path).entries.any? - return "raw" if @repository.blob_at(current_sha, path).image? - "blob" - end - - def current_sha - if @commit - @commit.id - elsif @repository && !@repository.empty? - if @ref - @repository.commit(@ref).try(:sha) - else - @repository.head_commit.sha - end - end - end - - # We will assume that if no ref exists we can point to master - def correct_ref - @ref ? @ref : "master" - end - private # Return +text+, truncated to +max_chars+ characters, excluding any HTML diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 66610a08a44..d0648da049b 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -10,6 +10,7 @@ module MergeRequests close_merge_requests reload_merge_requests + execute_mr_web_hooks comment_mr_with_commits true @@ -88,6 +89,20 @@ module MergeRequests end end + # Call merge request webhook with update branches + def execute_mr_web_hooks + merge_requests = @project.origin_merge_requests.opened + .where(source_branch: @branch_name) + .to_a + merge_requests += @fork_merge_requests.where(source_branch: @branch_name) + .to_a + merge_requests = filter_merge_requests(merge_requests) + + merge_requests.each do |merge_request| + execute_hooks(merge_request, 'update') + end + end + def filter_merge_requests(merge_requests) merge_requests.uniq.select(&:source_project) end diff --git a/doc/web_hooks/web_hooks.md b/doc/web_hooks/web_hooks.md index 01082555192..d140f3a457a 100644 --- a/doc/web_hooks/web_hooks.md +++ b/doc/web_hooks/web_hooks.md @@ -143,7 +143,7 @@ X-Gitlab-Event: Issue Hook ## Merge request events -Triggered when a new merge request is created or an existing merge request was updated/merged/closed. +Triggered when a new merge request is created, an existing merge request was updated/merged/closed or a commit is added in the source branch. **Request header**: diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 63294aa54c0..cc68416d5fc 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -15,6 +15,7 @@ module Gitlab autoload :IssueReferenceFilter, 'gitlab/markdown/issue_reference_filter' autoload :LabelReferenceFilter, 'gitlab/markdown/label_reference_filter' autoload :MergeRequestReferenceFilter, 'gitlab/markdown/merge_request_reference_filter' + autoload :RelativeLinkFilter, 'gitlab/markdown/relative_link_filter' autoload :SanitizationFilter, 'gitlab/markdown/sanitization_filter' autoload :SnippetReferenceFilter, 'gitlab/markdown/snippet_reference_filter' autoload :TableOfContentsFilter, 'gitlab/markdown/table_of_contents_filter' @@ -64,7 +65,12 @@ module Gitlab current_user: current_user, only_path: options[:reference_only_path], project: project, - reference_class: html_options[:class] + reference_class: html_options[:class], + + # RelativeLinkFilter + ref: @ref, + requested_path: @path, + project_wiki: @project_wiki } result = pipeline.call(text, context) @@ -91,6 +97,7 @@ module Gitlab [ Gitlab::Markdown::SanitizationFilter, + Gitlab::Markdown::RelativeLinkFilter, Gitlab::Markdown::EmojiFilter, Gitlab::Markdown::TableOfContentsFilter, Gitlab::Markdown::AutolinkFilter, diff --git a/lib/gitlab/markdown/relative_link_filter.rb b/lib/gitlab/markdown/relative_link_filter.rb new file mode 100644 index 00000000000..deb302c88e1 --- /dev/null +++ b/lib/gitlab/markdown/relative_link_filter.rb @@ -0,0 +1,114 @@ +require 'html/pipeline/filter' +require 'uri' + +module Gitlab + module Markdown + # HTML filter that "fixes" relative links to files in a repository. + # + # Context options: + # :commit + # :project + # :project_wiki + # :requested_path + # :ref + class RelativeLinkFilter < HTML::Pipeline::Filter + + def call + if linkable_files? + doc.search('a').each do |el| + process_link_attr el.attribute('href') + end + + doc.search('img').each do |el| + process_link_attr el.attribute('src') + end + end + + doc + end + + protected + + def linkable_files? + context[:project_wiki].nil? && repository.try(:exists?) && !repository.empty? + end + + def process_link_attr(html_attr) + return if html_attr.blank? + + uri = URI(html_attr.value) + if uri.relative? && uri.path.present? + html_attr.value = rebuild_relative_uri(uri).to_s + end + end + + def rebuild_relative_uri(uri) + file_path = relative_file_path(uri.path) + + uri.path = [ + relative_url_root, + context[:project].path_with_namespace, + path_type(file_path), + ref || 'master', # assume that if no ref exists we can point to master + file_path + ].compact.join('/').squeeze('/').chomp('/') + + uri + end + + def relative_file_path(path) + nested_path = build_nested_path(path, context[:requested_path]) + file_exists?(nested_path) ? nested_path : path + end + + # Covering a special case, when the link is referencing file in the same + # directory. + # If we are at doc/api/README.md and the README.md contains relative + # links like [Users](users.md), this takes the request + # path(doc/api/README.md) and replaces the README.md with users.md so the + # path looks like doc/api/users.md. + # If we are at doc/api and the README.md shown in below the tree view + # this takes the request path(doc/api) and adds users.md so the path + # looks like doc/api/users.md + def build_nested_path(path, request_path) + return request_path if path.empty? + return path unless request_path + + parts = request_path.split('/') + parts.pop if path_type(request_path) != 'tree' + parts.push(path).join('/') + end + + def file_exists?(path) + return false if path.nil? + repository.blob_at(current_sha, path).present? || + repository.tree(current_sha, path).entries.any? + end + + # Check if the path is pointing to a directory(tree) or a file(blob) + # eg. doc/api is directory and doc/README.md is file. + def path_type(path) + return 'tree' if repository.tree(current_sha, path).entries.any? + return 'raw' if repository.blob_at(current_sha, path).try(:image?) + 'blob' + end + + def current_sha + context[:commit].try(:id) || + ref ? repository.commit(ref).try(:sha) : repository.head_commit.sha + end + + def relative_url_root + Gitlab.config.gitlab.relative_url_root.presence || '/' + end + + def ref + context[:ref] + end + + def repository + context[:project].try(:repository) + end + end + end +end diff --git a/lib/redcarpet/render/gitlab_html.rb b/lib/redcarpet/render/gitlab_html.rb index bea66e6cdc1..15eb6effe08 100644 --- a/lib/redcarpet/render/gitlab_html.rb +++ b/lib/redcarpet/render/gitlab_html.rb @@ -36,10 +36,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML end def postprocess(full_document) - unless @template.instance_variable_get("@project_wiki") || @project.nil? - full_document = h.create_relative_links(full_document) - end - h.gfm_with_options(full_document, @options) end end diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 2f67879efdc..7d0335c2320 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -96,79 +96,6 @@ describe GitlabMarkdownHelper do end end - describe "#markdown" do - # TODO (rspeicher): These belong in a relative link filter spec - context 'relative links' do - context 'with a valid repository' do - before do - @repository = project.repository - @ref = 'markdown' - end - - it "should handle relative urls for a file in master" do - actual = "[GitLab API doc](doc/api/README.md)\n" - expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls for a file in master with an anchor" do - actual = "[GitLab API doc](doc/api/README.md#section)\n" - expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md#section\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should not handle relative urls for the current file with an anchor" do - actual = "[GitLab API doc](#section)\n" - expected = "<p><a href=\"#section\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls for a directory in master" do - actual = "[GitLab API doc](doc/api)\n" - expected = "<p><a href=\"/#{project.path_with_namespace}/tree/#{@ref}/doc/api\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle absolute urls" do - actual = "[GitLab](https://www.gitlab.com)\n" - expected = "<p><a href=\"https://www.gitlab.com\">GitLab</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls in reference links for a file in master" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should handle relative urls in reference links for a directory in master" do - actual = "[GitLab API doc directory][GitLab readmes]\n [GitLab readmes]: doc/api/\n" - expected = "<p><a href=\"/#{project.path_with_namespace}/tree/#{@ref}/doc/api\">GitLab API doc directory</a></p>\n" - expect(markdown(actual)).to match(expected) - end - - it "should not handle malformed relative urls in reference links for a file in master" do - actual = "[GitLab readme]: doc/api/README.md\n" - expected = "" - expect(markdown(actual)).to match(expected) - end - end - - context 'with an empty repository' do - before do - @project = create(:empty_project) - @repository = @project.repository - end - - it "should not touch relative urls" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "<p><a href=\"doc/api/README.md\">GitLab API doc</a></p>\n" - expect(markdown(actual)).to match(expected) - end - end - end - end - describe '#render_wiki_content' do before do @wiki = double('WikiPage') diff --git a/spec/lib/gitlab/markdown/relative_link_filter_spec.rb b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb new file mode 100644 index 00000000000..38cf567d73f --- /dev/null +++ b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb @@ -0,0 +1,120 @@ +require 'spec_helper' + +module Gitlab::Markdown + describe RelativeLinkFilter do + include ActionView::Helpers::TagHelper + + let!(:project) { create(:project) } + + let(:commit) { project.commit } + let(:project_path) { project.path_with_namespace } + let(:repository) { project.repository } + let(:ref) { 'markdown' } + + let(:project_wiki) { nil } + let(:requested_path) { '/' } + let(:blob) { RepoHelpers.sample_blob } + + let(:context) do + { + commit: commit, + project: project, + project_wiki: project_wiki, + requested_path: requested_path, + ref: ref + } + end + + + shared_examples :preserve_unchanged do + + it "should not modify any relative url in anchor" do + doc = tag(:a, href: 'README.md') + expect( filter(doc) ).to match '"README.md"' + end + + it "should not modify any relative url in image" do + doc = tag(:img, src: 'files/images/logo-black.png') + expect( filter(doc) ).to match '"files/images/logo-black.png"' + end + end + + shared_examples :relative_to_requested do + + it "should rebuild url relative to the requested path" do + expect( filter(tag(:a, href: 'users.md')) ).to \ + match %("/#{project_path}/blob/#{ref}/doc/api/users.md") + end + end + + + context "with a project_wiki" do + let(:project_wiki) { double('ProjectWiki') } + + include_examples :preserve_unchanged + end + + context "without a repository" do + let!(:project) { create(:empty_project) } + + include_examples :preserve_unchanged + end + + context "with an empty repository" do + let!(:project) { create(:project_empty_repo) } + + include_examples :preserve_unchanged + end + + + context "with a valid repository" do + + it "should rebuild relative url for a file in the repo" do + expect( filter(tag(:a, href: 'doc/api/README.md')) ).to \ + match %("/#{project_path}/blob/#{ref}/doc/api/README.md") + end + + it "should rebuild relative url for a file in the repo with an anchor" do + expect( filter(tag(:a, href: 'README.md#section')) ).to \ + match %("/#{project_path}/blob/#{ref}/README.md#section") + end + + it "should rebuild relative url for a directory in the repo" do + expect( filter(tag(:a, href: 'doc/api/')) ).to \ + match %("/#{project_path}/tree/#{ref}/doc/api") + end + + it "should rebuild relative url for an image in the repo" do + expect( filter(tag(:img, src: 'files/images/logo-black.png')) ).to \ + match %("/#{project_path}/raw/#{ref}/files/images/logo-black.png") + end + + it "should not modify relative url with an anchor only" do + doc = tag(:a, href: '#section-1') + expect( filter(doc) ).to match %("#section-1") + end + + it "should not modify absolute url" do + expect( filter(tag(:a, href: 'http://example.org')) ).to \ + match %("http://example.org") + end + + context "when requested path is a file in the repo" do + let(:requested_path) { 'doc/api/README.md' } + + include_examples :relative_to_requested + end + + context "when requested path is a directory in the repo" do + let(:requested_path) { 'doc/api' } + + include_examples :relative_to_requested + end + end + + + def filter(doc) + described_class.call(doc, context).to_s + end + end +end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 879df0c9c67..0f9b65678df 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -30,11 +30,18 @@ describe MergeRequests::RefreshService do end context 'push to origin repo source branch' do + let(:refresh_service) { service.new(@project, @user) } before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') + allow(refresh_service).to receive(:execute_hooks) + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end + it 'should execute hooks with update action' do + expect(refresh_service).to have_received(:execute_hooks). + with(@merge_request, 'update') + end + it { expect(@merge_request.notes).not_to be_empty } it { expect(@merge_request).to be_open } it { expect(@fork_merge_request).to be_open } @@ -54,11 +61,18 @@ describe MergeRequests::RefreshService do end context 'push to fork repo source branch' do + let(:refresh_service) { service.new(@fork_project, @user) } before do - service.new(@fork_project, @user).execute(@oldrev, @newrev, 'refs/heads/master') + allow(refresh_service).to receive(:execute_hooks) + refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs end + it 'should execute hooks with update action' do + expect(refresh_service).to have_received(:execute_hooks). + with(@fork_merge_request, 'update') + end + it { expect(@merge_request.notes).to be_empty } it { expect(@merge_request).to be_open } it { expect(@fork_merge_request.notes.last.note).to include('Added 4 commits') } |