From 879be42862e05761c5b57d31961ed1a978de957d Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 14:21:20 -0400 Subject: [PATCH 01/22] Initialize the references result Hash in ReferenceFilter --- lib/gitlab/markdown/reference_filter.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/gitlab/markdown/reference_filter.rb b/lib/gitlab/markdown/reference_filter.rb index ef4aa408a7e..efd7ea16fcc 100644 --- a/lib/gitlab/markdown/reference_filter.rb +++ b/lib/gitlab/markdown/reference_filter.rb @@ -12,7 +12,15 @@ module Gitlab # :reference_class - Custom CSS class added to reference links. # :only_path - Generate path-only links. # + # Results: + # :references - A Hash of references that were found and replaced. class ReferenceFilter < HTML::Pipeline::Filter + def initialize(*args) + super + + result[:references] = Hash.new { |hash, type| hash[type] = [] } + end + def escape_once(html) ERB::Util.html_escape_once(html) end @@ -29,6 +37,14 @@ module Gitlab context[:project] end + # Add a reference to the pipeline's result Hash + # + # type - Singular Symbol reference type (e.g., :issue, :user, etc.) + # value - Object to add + def push_result(type, value) + result[:references][type].push(value) + end + def reference_class(type) "gfm gfm-#{type} #{context[:reference_class]}".strip end -- GitLab From 85082de780b500a3469db7867cd938154fc0622f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 15:27:29 -0400 Subject: [PATCH 02/22] Add `pipeline_result` to ReferenceFilterSpecHelper --- spec/support/reference_filter_spec_helper.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/support/reference_filter_spec_helper.rb b/spec/support/reference_filter_spec_helper.rb index bcee5715cad..41253811490 100644 --- a/spec/support/reference_filter_spec_helper.rb +++ b/spec/support/reference_filter_spec_helper.rb @@ -35,6 +35,20 @@ module ReferenceFilterSpecHelper described_class.call(html, contexts) end + # Run text through HTML::Pipeline with the current filter and return the + # result Hash + # + # body - String text to run through the pipeline + # contexts - Hash context for the filter. (default: {project: project}) + # + # Returns the Hash of the pipeline result + def pipeline_result(body, contexts = {}) + contexts.reverse_merge!(project: project) + + pipeline = HTML::Pipeline.new([described_class], contexts) + pipeline.call(body) + end + def allow_cross_reference! allow_any_instance_of(described_class). to receive(:user_can_reference_project?).and_return(true) -- GitLab From a6defd157641b102c9ea4cabe99f0a386d661d80 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 15:56:45 -0400 Subject: [PATCH 03/22] Add results to reference filters --- .../markdown/commit_range_reference_filter.rb | 2 + .../markdown/commit_reference_filter.rb | 2 + lib/gitlab/markdown/issue_reference_filter.rb | 3 + lib/gitlab/markdown/label_reference_filter.rb | 6 +- .../merge_request_reference_filter.rb | 2 + .../markdown/snippet_reference_filter.rb | 2 + lib/gitlab/markdown/user_reference_filter.rb | 7 ++ .../commit_range_reference_filter_spec.rb | 10 +++ .../markdown/commit_reference_filter_spec.rb | 10 +++ .../markdown/issue_reference_filter_spec.rb | 12 +++- .../markdown/label_reference_filter_spec.rb | 5 ++ .../merge_request_reference_filter_spec.rb | 10 +++ .../markdown/snippet_reference_filter_spec.rb | 10 +++ .../markdown/user_reference_filter_spec.rb | 66 ++++++++++++++----- 14 files changed, 129 insertions(+), 18 deletions(-) diff --git a/lib/gitlab/markdown/commit_range_reference_filter.rb b/lib/gitlab/markdown/commit_range_reference_filter.rb index baa97bee9bf..5e5e7adc238 100644 --- a/lib/gitlab/markdown/commit_range_reference_filter.rb +++ b/lib/gitlab/markdown/commit_range_reference_filter.rb @@ -59,6 +59,8 @@ module Gitlab from_id, to_id = split_commit_range(commit_range) if valid_range?(project, from_id, to_id) + push_result(:commit_range, [commit(from_id), commit(to_id)]) + url = url_for_commit_range(project, from_id, to_id) title = "Commits #{from_id} through #{to_id}" diff --git a/lib/gitlab/markdown/commit_reference_filter.rb b/lib/gitlab/markdown/commit_reference_filter.rb index 66598127f6e..41fb03cdd41 100644 --- a/lib/gitlab/markdown/commit_reference_filter.rb +++ b/lib/gitlab/markdown/commit_reference_filter.rb @@ -48,6 +48,8 @@ module Gitlab project = self.project_from_ref(project_ref) if commit = commit_from_ref(project, commit_ref) + push_result(:commit, commit) + url = url_for_commit(project, commit) title = escape_once(commit.link_title) diff --git a/lib/gitlab/markdown/issue_reference_filter.rb b/lib/gitlab/markdown/issue_reference_filter.rb index c9267cc3e9d..4b360369d37 100644 --- a/lib/gitlab/markdown/issue_reference_filter.rb +++ b/lib/gitlab/markdown/issue_reference_filter.rb @@ -48,6 +48,9 @@ module Gitlab project = self.project_from_ref(project_ref) if project && project.issue_exists?(issue) + # FIXME (rspeicher): Law of Demeter + push_result(:issue, project.issues.where(iid: issue).first) + url = url_for_issue(issue, project, only_path: context[:only_path]) title = escape_once("Issue: #{title_for_issue(issue, project)}") diff --git a/lib/gitlab/markdown/label_reference_filter.rb b/lib/gitlab/markdown/label_reference_filter.rb index 4c21192c0d3..a357f28458d 100644 --- a/lib/gitlab/markdown/label_reference_filter.rb +++ b/lib/gitlab/markdown/label_reference_filter.rb @@ -52,11 +52,13 @@ module Gitlab params = label_params(id, name) if label = project.labels.find_by(params) - url = url_for_label(project, label) + push_result(:label, label) + url = url_for_label(project, label) klass = reference_class(:label) - %(#{render_colored_label(label)}) + %(#{render_colored_label(label)}) else match end diff --git a/lib/gitlab/markdown/merge_request_reference_filter.rb b/lib/gitlab/markdown/merge_request_reference_filter.rb index 40239523cda..7c28fe112ef 100644 --- a/lib/gitlab/markdown/merge_request_reference_filter.rb +++ b/lib/gitlab/markdown/merge_request_reference_filter.rb @@ -48,6 +48,8 @@ module Gitlab project = self.project_from_ref(project_ref) if project && merge_request = project.merge_requests.find_by(iid: id) + push_result(:merge_request, merge_request) + title = escape_once("Merge Request: #{merge_request.title}") klass = reference_class(:merge_request) diff --git a/lib/gitlab/markdown/snippet_reference_filter.rb b/lib/gitlab/markdown/snippet_reference_filter.rb index ada67de992b..64a0a2696f7 100644 --- a/lib/gitlab/markdown/snippet_reference_filter.rb +++ b/lib/gitlab/markdown/snippet_reference_filter.rb @@ -48,6 +48,8 @@ module Gitlab project = self.project_from_ref(project_ref) if project && snippet = project.snippets.find_by(id: id) + push_result(:snippet, snippet) + title = escape_once("Snippet: #{snippet.title}") klass = reference_class(:snippet) diff --git a/lib/gitlab/markdown/user_reference_filter.rb b/lib/gitlab/markdown/user_reference_filter.rb index 5fc8ed55fe2..d3af206335d 100644 --- a/lib/gitlab/markdown/user_reference_filter.rb +++ b/lib/gitlab/markdown/user_reference_filter.rb @@ -44,18 +44,25 @@ module Gitlab klass = reference_class(:project_member) if user == 'all' + # FIXME (rspeicher): Law of Demeter + push_result(:user, project.team.members.flatten) + url = link_to_all(project) %(@#{user}) elsif namespace = Namespace.find_by(path: user) if namespace.is_a?(Group) if user_can_reference_group?(namespace) + push_result(:user, namespace.users) + url = group_url(user, only_path: context[:only_path]) %(@#{user}) else match end else + push_result(:user, namespace.owner) + url = user_url(user, only_path: context[:only_path]) %(@#{user}) end diff --git a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb index f0804ce0056..6bfe7bb3615 100644 --- a/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb @@ -81,6 +81,11 @@ module Gitlab::Markdown expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_compare_url(project.namespace, project, from: commit1.id, to: commit2.id, only_path: true) end + + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit_range]).not_to be_empty + end end context 'cross-project reference' do @@ -112,6 +117,11 @@ module Gitlab::Markdown exp = act = "Fixed #{project2.path_with_namespace}##{commit1.id}...#{commit2.id.reverse}" expect(filter(act).to_html).to eq exp end + + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit_range]).not_to be_empty + end end context 'when user cannot access reference' do diff --git a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb index f792d7f696e..5efda4f9aa4 100644 --- a/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/commit_reference_filter_spec.rb @@ -75,6 +75,11 @@ module Gitlab::Markdown expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_commit_url(project.namespace, project, reference, only_path: true) end + + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit]).not_to be_empty + end end context 'cross-project reference' do @@ -102,6 +107,11 @@ module Gitlab::Markdown exp = act = "Committed #{project2.path_with_namespace}##{commit.id.reverse}" expect(filter(act).to_html).to eq exp end + + it 'adds to the results hash' do + result = pipeline_result("See #{reference}") + expect(result[:references][:commit]).not_to be_empty + end end context 'when user cannot access reference' do diff --git a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb index f95b37d6954..393bf32e196 100644 --- a/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/issue_reference_filter_spec.rb @@ -34,7 +34,7 @@ module Gitlab::Markdown end it 'links to a valid reference' do - doc = filter("See #{reference}") + doc = filter("Fixed #{reference}") expect(doc.css('a').first.attr('href')). to eq helper.url_for_issue(issue.iid, project) @@ -81,6 +81,11 @@ module Gitlab::Markdown expect(link).not_to match %r(https?://) expect(link).to eq helper.url_for_issue(issue.iid, project, only_path: true) end + + it 'adds to the results hash' do + result = pipeline_result("Fixed #{reference}") + expect(result[:references][:issue]).to eq [issue] + end end context 'cross-project reference' do @@ -117,6 +122,11 @@ module Gitlab::Markdown expect(filter(act).to_html).to eq exp end + + it 'adds to the results hash' do + result = pipeline_result("Fixed #{reference}") + expect(result[:references][:issue]).to eq [issue] + end end context 'when user cannot access reference' do diff --git a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb index c84e568e172..9f898837466 100644 --- a/spec/lib/gitlab/markdown/label_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/label_reference_filter_spec.rb @@ -39,6 +39,11 @@ module Gitlab::Markdown expect(link).to eq urls.namespace_project_issues_url(project.namespace, project, label_name: label.name, only_path: true) end + it 'adds to the results hash' do + result = pipeline_result("Label #{reference}") + expect(result[:references][:label]).to eq [label] + end + describe 'label span element' do it 'includes default classes' do doc = filter("Label #{reference}") diff --git a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb index 0f66442269b..d6e745114f2 100644 --- a/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb @@ -69,6 +69,11 @@ module Gitlab::Markdown expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_merge_request_url(project.namespace, project, merge, only_path: true) end + + it 'adds to the results hash' do + result = pipeline_result("Merge #{reference}") + expect(result[:references][:merge_request]).to eq [merge] + end end context 'cross-project reference' do @@ -98,6 +103,11 @@ module Gitlab::Markdown expect(filter(act).to_html).to eq exp end + + it 'adds to the results hash' do + result = pipeline_result("Merge #{reference}") + expect(result[:references][:merge_request]).to eq [merge] + end end context 'when user cannot access reference' do diff --git a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb index 79533a90b55..a4b331157af 100644 --- a/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb @@ -68,6 +68,11 @@ module Gitlab::Markdown expect(link).not_to match %r(https?://) expect(link).to eq urls.namespace_project_snippet_url(project.namespace, project, snippet, only_path: true) end + + it 'adds to the results hash' do + result = pipeline_result("Snippet #{reference}") + expect(result[:references][:snippet]).to eq [snippet] + end end context 'cross-project reference' do @@ -96,6 +101,11 @@ module Gitlab::Markdown expect(filter(act).to_html).to eq exp end + + it 'adds to the results hash' do + result = pipeline_result("Snippet #{reference}") + expect(result[:references][:snippet]).to eq [snippet] + end end context 'when user cannot access reference' do diff --git a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb index a5eb927072e..f5a14367311 100644 --- a/spec/lib/gitlab/markdown/user_reference_filter_spec.rb +++ b/spec/lib/gitlab/markdown/user_reference_filter_spec.rb @@ -24,9 +24,29 @@ module Gitlab::Markdown end end + context 'mentioning @all' do + before do + project.team << [project.creator, :developer] + end + + it 'supports a special @all mention' do + doc = filter("Hey @all") + expect(doc.css('a').length).to eq 1 + expect(doc.css('a').first.attr('href')) + .to eq urls.namespace_project_url(project.namespace, project) + end + + it 'adds to the results hash' do + result = pipeline_result('Hey @all') + expect(result[:references][:user]).to eq [[project.creator]] + end + end + context 'mentioning a user' do + let(:reference) { "@#{user.username}" } + it 'links to a User' do - doc = filter("Hey @#{user.username}") + doc = filter("Hey #{reference}") expect(doc.css('a').first.attr('href')).to eq urls.user_url(user) end @@ -45,22 +65,45 @@ module Gitlab::Markdown doc = filter("Hey @#{user.username}") expect(doc.css('a').length).to eq 1 end + + it 'adds to the results hash' do + result = pipeline_result("Hey #{reference}") + expect(result[:references][:user]).to eq [user] + end end context 'mentioning a group' do let(:group) { create(:group) } let(:user) { create(:user) } - it 'links to a Group that the current user can read' do - group.add_user(user, Gitlab::Access::DEVELOPER) + let(:reference) { "@#{group.name}" } + + context 'that the current user can read' do + before do + group.add_user(user, Gitlab::Access::DEVELOPER) + end - doc = filter("Hey @#{group.name}", current_user: user) - expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) + it 'links to the Group' do + doc = filter("Hey #{reference}", current_user: user) + expect(doc.css('a').first.attr('href')).to eq urls.group_url(group) + end + + it 'adds to the results hash' do + result = pipeline_result("Hey #{reference}", current_user: user) + expect(result[:references][:user]).to eq [group.users] + end end - it 'ignores references to a Group that the current user cannot read' do - doc = filter("Hey @#{group.name}", current_user: user) - expect(doc.to_html).to eq "Hey @#{group.name}" + context 'that the current user cannot read' do + it 'ignores references to the Group' do + doc = filter("Hey #{reference}", current_user: user) + expect(doc.to_html).to eq "Hey #{reference}" + end + + it 'does not add to the results hash' do + result = pipeline_result("Hey #{reference}", current_user: user) + expect(result[:references][:user]).to eq [] + end end end @@ -70,13 +113,6 @@ module Gitlab::Markdown expect(doc.to_html).to match(/\(@#{user.username}<\/a>\.\)/) end - it 'supports a special @all mention' do - doc = filter("Hey @all") - expect(doc.css('a').length).to eq 1 - expect(doc.css('a').first.attr('href')) - .to eq urls.namespace_project_url(project.namespace, project) - end - it 'includes default classes' do doc = filter("Hey @#{user.username}") expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-project_member' -- GitLab From 8f8a8ab32bca8fdc79d7a5115eabbd015dd44c02 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 16:01:00 -0400 Subject: [PATCH 04/22] Refactor ReferenceExtractor to use pipeline filters --- lib/gitlab/reference_extractor.rb | 155 +++++--------------- spec/lib/gitlab/reference_extractor_spec.rb | 74 ---------- 2 files changed, 36 insertions(+), 193 deletions(-) diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 949dd5d26b1..1f8558f5ad0 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -8,151 +8,68 @@ module Gitlab @current_user = current_user end - def can?(user, action, subject) - Ability.abilities.allowed?(user, action, subject) - end - def analyze(text) - text = text.dup - - # Remove preformatted/code blocks so that references are not included - text.gsub!(/^```.*?^```/m, '') - text.gsub!(/[^`]`[^`]*?`[^`]/, '') - - @references = Hash.new { |hash, type| hash[type] = [] } - parse_references(text) + @_text = text.dup end - # Given a valid project, resolve the extracted identifiers of the requested type to - # model objects. - def users - references[:user].uniq.map do |project, identifier| - if identifier == "all" - project.team.members.flatten - elsif namespace = Namespace.find_by(path: identifier) - if namespace.is_a?(Group) - namespace.users if can?(current_user, :read_group, namespace) - else - namespace.owner - end - end - end.flatten.compact.uniq + result = pipeline_result(:user) + result[:references][:user].flatten.compact.uniq end def labels - references[:label].uniq.map do |project, identifier| - project.labels.where(id: identifier).first - end.compact.uniq + result = pipeline_result(:label) + result[:references][:label].compact.uniq end def issues - references[:issue].uniq.map do |project, identifier| - if project.default_issues_tracker? - project.issues.where(iid: identifier).first - end - end.compact.uniq + # TODO (rspeicher): What about external issues? + + result = pipeline_result(:issue) + result[:references][:issue].compact.uniq end def merge_requests - references[:merge_request].uniq.map do |project, identifier| - project.merge_requests.where(iid: identifier).first - end.compact.uniq + result = pipeline_result(:merge_request) + result[:references][:merge_request].compact.uniq end def snippets - references[:snippet].uniq.map do |project, identifier| - project.snippets.where(id: identifier).first - end.compact.uniq + result = pipeline_result(:snippet) + result[:references][:snippet].compact.uniq end def commits - references[:commit].uniq.map do |project, identifier| - repo = project.repository - repo.commit(identifier) if repo - end.compact.uniq + result = pipeline_result(:commit) + result[:references][:commit].compact.uniq end def commit_ranges - references[:commit_range].uniq.map do |project, identifier| - repo = project.repository - if repo - from_id, to_id = identifier.split(/\.{2,3}/, 2) - [repo.commit(from_id), repo.commit(to_id)] - end - end.compact.uniq + result = pipeline_result(:commit_range) + result[:references][:commit_range].compact.uniq end private - NAME_STR = Gitlab::Regex::NAMESPACE_REGEX_STR - PROJ_STR = "(?#{NAME_STR}/#{NAME_STR})" - - REFERENCE_PATTERN = %r{ - (?\W)? # Prefix - ( # Reference - @(?#{NAME_STR}) # User name - |~(?