diff options
author | Felipe Artur <felipefac@gmail.com> | 2016-10-06 19:05:27 -0300 |
---|---|---|
committer | Felipe Artur <felipefac@gmail.com> | 2016-10-19 13:46:04 -0200 |
commit | 8e4301d982ce28d80e711865ac294a98ddce3ec6 (patch) | |
tree | 1f6d01b142ca952f3cd3098336fbb0762fe0a8dc | |
parent | f560849941dcb4408350c6c24034f845d3986782 (diff) | |
download | gitlab-ce-8e4301d982ce28d80e711865ac294a98ddce3ec6.tar.gz |
Prevent wrong markdown on issue ids when project has Jira service activatedissue_828
-rw-r--r-- | CHANGELOG.md | 1 | ||||
-rw-r--r-- | app/models/external_issue.rb | 5 | ||||
-rw-r--r-- | app/models/project.rb | 4 | ||||
-rw-r--r-- | app/models/project_services/issue_tracker_service.rb | 6 | ||||
-rw-r--r-- | app/models/project_services/jira_service.rb | 5 | ||||
-rw-r--r-- | lib/banzai/filter/abstract_reference_filter.rb | 16 | ||||
-rw-r--r-- | lib/banzai/filter/external_issue_reference_filter.rb | 29 | ||||
-rw-r--r-- | lib/banzai/filter/issue_reference_filter.rb | 8 | ||||
-rw-r--r-- | spec/lib/banzai/filter/external_issue_reference_filter_spec.rb | 72 | ||||
-rw-r--r-- | spec/lib/banzai/filter/issue_reference_filter_spec.rb | 17 | ||||
-rw-r--r-- | spec/models/external_issue_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/project_services/jira_service_spec.rb | 9 | ||||
-rw-r--r-- | spec/models/project_services/redmine_service_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 60 | ||||
-rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 12 | ||||
-rw-r--r-- | spec/support/issue_tracker_service_shared_example.rb | 15 |
16 files changed, 199 insertions, 83 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index f4d317fd1db..c4fcca1b016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Please view this file on the master branch, on stable branches it's out of date. - Update runner version only when updating contacted_at - Add link from system note to compare with previous version - Use gitlab-shell v3.6.6 + - Ignore references to internal issues when using external issues tracker - Ability to resolve merge request conflicts with editor !6374 - Add `/projects/visible` API endpoint (Ben Boeckel) - Fix centering of custom header logos (Ashley Dumaine) diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb index b7894c99846..fd9a8c1b8b7 100644 --- a/app/models/external_issue.rb +++ b/app/models/external_issue.rb @@ -29,11 +29,6 @@ class ExternalIssue @project end - # Pattern used to extract `JIRA-123` issue references from text - def self.reference_pattern - @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)} - end - def to_reference(_from_project = nil) id end diff --git a/app/models/project.rb b/app/models/project.rb index db7301219e5..a645c9b29cc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -664,6 +664,10 @@ class Project < ActiveRecord::Base end end + def issue_reference_pattern + issues_tracker.reference_pattern + end + def default_issues_tracker? !external_issue_tracker end diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index d1df6d0292f..b26ddd518d7 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -3,6 +3,12 @@ class IssueTrackerService < Service default_value_for :category, 'issue_tracker' + # Pattern used to extract links from comments + # Override this method on services that uses different patterns + def reference_pattern + @reference_pattern ||= %r{(\b[A-Z][A-Z0-9_]+-|#{Issue.reference_prefix})(?<issue>\d+)} + end + def default? default end diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 97bcbacf2b9..f81b66fd219 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -13,6 +13,11 @@ class JiraService < IssueTrackerService before_update :reset_password + # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1 + def reference_pattern + @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)} + end + def reset_password # don't reset the password if a new one is provided if api_url_changed? && !password_touched? diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index affe34394c2..cb213a76a05 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -208,8 +208,12 @@ module Banzai @references_per_project ||= begin refs = Hash.new { |hash, key| hash[key] = Set.new } - regex = Regexp.union(object_class.reference_pattern, - object_class.link_reference_pattern) + regex = + if uses_reference_pattern? + Regexp.union(object_class.reference_pattern, object_class.link_reference_pattern) + else + object_class.link_reference_pattern + end nodes.each do |node| node.to_html.scan(regex) do @@ -295,6 +299,14 @@ module Banzai value end end + + # There might be special cases like filters + # that should ignore reference pattern + # eg: IssueReferenceFilter when using a external issues tracker + # In those cases this method should be overridden on the filter subclass + def uses_reference_pattern? + true + end end end end diff --git a/lib/banzai/filter/external_issue_reference_filter.rb b/lib/banzai/filter/external_issue_reference_filter.rb index eaa702952cc..0d20be557a0 100644 --- a/lib/banzai/filter/external_issue_reference_filter.rb +++ b/lib/banzai/filter/external_issue_reference_filter.rb @@ -8,7 +8,7 @@ module Banzai # Public: Find `JIRA-123` issue references in text # - # ExternalIssueReferenceFilter.references_in(text) do |match, issue| + # ExternalIssueReferenceFilter.references_in(text, pattern) do |match, issue| # "<a href=...>##{issue}</a>" # end # @@ -17,8 +17,8 @@ module Banzai # Yields the String match and the String issue reference. # # Returns a String replaced with the return of the block. - def self.references_in(text) - text.gsub(ExternalIssue.reference_pattern) do |match| + def self.references_in(text, pattern) + text.gsub(pattern) do |match| yield match, $~[:issue] end end @@ -27,7 +27,7 @@ module Banzai # Early return if the project isn't using an external tracker return doc if project.nil? || default_issues_tracker? - ref_pattern = ExternalIssue.reference_pattern + ref_pattern = issue_reference_pattern ref_start_pattern = /\A#{ref_pattern}\z/ each_node do |node| @@ -60,7 +60,7 @@ module Banzai def issue_link_filter(text, link_text: nil) project = context[:project] - self.class.references_in(text) do |match, id| + self.class.references_in(text, issue_reference_pattern) do |match, id| ExternalIssue.new(id, project) url = url_for_issue(id, project, only_path: context[:only_path]) @@ -82,18 +82,21 @@ module Banzai end def default_issues_tracker? - if RequestStore.active? - default_issues_tracker_cache[project.id] ||= - project.default_issues_tracker? - else - project.default_issues_tracker? - end + external_issues_cached(:default_issues_tracker?) + end + + def issue_reference_pattern + external_issues_cached(:issue_reference_pattern) end private - def default_issues_tracker_cache - RequestStore[:banzai_default_issues_tracker_cache] ||= {} + def external_issues_cached(attribute) + return project.public_send(attribute) unless RequestStore.active? + + cached_attributes = RequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} } + cached_attributes[project.id][attribute] = project.public_send(attribute) if cached_attributes[project.id][attribute].nil? + cached_attributes[project.id][attribute] end end end diff --git a/lib/banzai/filter/issue_reference_filter.rb b/lib/banzai/filter/issue_reference_filter.rb index 54c5f9a71a4..4d1bc687696 100644 --- a/lib/banzai/filter/issue_reference_filter.rb +++ b/lib/banzai/filter/issue_reference_filter.rb @@ -4,6 +4,10 @@ module Banzai # issues that do not exist are ignored. # # This filter supports cross-project references. + # + # When external issues tracker like Jira is activated we should not + # use issue reference pattern, but we should still be able + # to reference issues from other GitLab projects. class IssueReferenceFilter < AbstractReferenceFilter self.reference_type = :issue @@ -11,6 +15,10 @@ module Banzai Issue end + def uses_reference_pattern? + context[:project].default_issues_tracker? + end + def find_object(project, iid) issues_per_project[project][iid] end diff --git a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb index 7116c09fb21..2f9343fadaf 100644 --- a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb @@ -7,12 +7,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do IssuesHelper end - let(:project) { create(:jira_project) } - - context 'JIRA issue references' do - let(:issue) { ExternalIssue.new('JIRA-123', project) } - let(:reference) { issue.to_reference } - + shared_examples_for "external issue tracker" do it 'requires project context' do expect { described_class.call('') }.to raise_error(ArgumentError, /:project/) end @@ -20,6 +15,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do %w(pre code a style).each do |elem| it "ignores valid references contained inside '#{elem}' element" do exp = act = "<#{elem}>Issue #{reference}</#{elem}>" + expect(filter(act).to_html).to eq exp end end @@ -33,25 +29,30 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do it 'links to a valid reference' do doc = filter("Issue #{reference}") + issue_id = doc.css('a').first.attr("data-external-issue") + expect(doc.css('a').first.attr('href')) - .to eq helper.url_for_issue(reference, project) + .to eq helper.url_for_issue(issue_id, project) end it 'links to the external tracker' do doc = filter("Issue #{reference}") + link = doc.css('a').first.attr('href') + issue_id = doc.css('a').first.attr("data-external-issue") - expect(link).to eq "http://jira.example/browse/#{reference}" + expect(link).to eq(helper.url_for_issue(issue_id, project)) end it 'links with adjacent text' do doc = filter("Issue (#{reference}.)") + expect(doc.to_html).to match(/\(<a.+>#{reference}<\/a>\.\)/) end it 'includes a title attribute' do doc = filter("Issue #{reference}") - expect(doc.css('a').first.attr('title')).to eq "Issue in JIRA tracker" + expect(doc.css('a').first.attr('title')).to include("Issue in #{project.issues_tracker.title}") end it 'escapes the title attribute' do @@ -69,9 +70,60 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do it 'supports an :only_path context' do doc = filter("Issue #{reference}", only_path: true) + link = doc.css('a').first.attr('href') + issue_id = doc.css('a').first["data-external-issue"] + + expect(link).to eq helper.url_for_issue(issue_id, project, only_path: true) + end + + context 'with RequestStore enabled' do + let(:reference_filter) { HTML::Pipeline.new([described_class]) } + + before { allow(RequestStore).to receive(:active?).and_return(true) } + + it 'queries the collection on the first call' do + expect_any_instance_of(Project).to receive(:default_issues_tracker?).once.and_call_original + expect_any_instance_of(Project).to receive(:issue_reference_pattern).once.and_call_original + + not_cached = reference_filter.call("look for #{reference}", { project: project }) + + expect_any_instance_of(Project).not_to receive(:default_issues_tracker?) + expect_any_instance_of(Project).not_to receive(:issue_reference_pattern) + + cached = reference_filter.call("look for #{reference}", { project: project }) - expect(link).to eq helper.url_for_issue("#{reference}", project, only_path: true) + # Links must be the same + expect(cached[:output].css('a').first[:href]).to eq(not_cached[:output].css('a').first[:href]) + end + end + end + + context "redmine project" do + let(:project) { create(:redmine_project) } + let(:issue) { ExternalIssue.new("#123", project) } + let(:reference) { issue.to_reference } + + it_behaves_like "external issue tracker" + end + + context "jira project" do + let(:project) { create(:jira_project) } + let(:reference) { issue.to_reference } + + context "with right markdown" do + let(:issue) { ExternalIssue.new("JIRA-123", project) } + + it_behaves_like "external issue tracker" + end + + context "with wrong markdown" do + let(:issue) { ExternalIssue.new("#123", project) } + + it "ignores reference" do + exp = act = "Issue #{reference}" + expect(filter(act).to_html).to eq exp + end end end end diff --git a/spec/lib/banzai/filter/issue_reference_filter_spec.rb b/spec/lib/banzai/filter/issue_reference_filter_spec.rb index fce86a9b6ad..a2025672ad9 100644 --- a/spec/lib/banzai/filter/issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/issue_reference_filter_spec.rb @@ -25,9 +25,7 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do let(:reference) { issue.to_reference } it 'ignores valid references when using non-default tracker' do - expect_any_instance_of(described_class).to receive(:find_object). - with(project, issue.iid). - and_return(nil) + allow(project).to receive(:default_issues_tracker?).and_return(false) exp = act = "Issue #{reference}" expect(reference_filter(act).to_html).to eq exp @@ -199,19 +197,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do end end - context 'referencing external issues' do - let(:project) { create(:redmine_project) } - - it 'renders internal issue IDs as external issue links' do - doc = reference_filter('#1') - link = doc.css('a').first - - expect(link.attr('data-reference-type')).to eq('external_issue') - expect(link.attr('title')).to eq('Issue in Redmine') - expect(link.attr('data-external-issue')).to eq('1') - end - end - describe '#issues_per_Project' do context 'using an internal issue tracker' do it 'returns a Hash containing the issues per project' do diff --git a/spec/models/external_issue_spec.rb b/spec/models/external_issue_spec.rb index 4fc3b065592..ebba6e14578 100644 --- a/spec/models/external_issue_spec.rb +++ b/spec/models/external_issue_spec.rb @@ -10,21 +10,6 @@ describe ExternalIssue, models: true do it { is_expected.to include_module(Referable) } end - describe '.reference_pattern' do - it 'allows underscores in the project name' do - expect(ExternalIssue.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234' - end - - it 'allows numbers in the project name' do - expect(ExternalIssue.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234' - end - - it 'requires the project name to begin with A-Z' do - expect(ExternalIssue.reference_pattern.match('3EXT_EXT-1234')).to eq nil - expect(ExternalIssue.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234' - end - end - describe '#to_reference' do it 'returns a String reference to the object' do expect(issue.to_reference).to eq issue.id diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb index b48a3176007..6ff32aea018 100644 --- a/spec/models/project_services/jira_service_spec.rb +++ b/spec/models/project_services/jira_service_spec.rb @@ -30,6 +30,15 @@ describe JiraService, models: true do end end + describe '#reference_pattern' do + it_behaves_like 'allows project key on reference pattern' + + it 'does not allow # on the code' do + expect(subject.reference_pattern.match('#123')).to be_nil + expect(subject.reference_pattern.match('1#23#12')).to be_nil + end + end + describe "Execute" do let(:user) { create(:user) } let(:project) { create(:project) } diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb index b8679cd2563..0a7b237a051 100644 --- a/spec/models/project_services/redmine_service_spec.rb +++ b/spec/models/project_services/redmine_service_spec.rb @@ -26,4 +26,12 @@ describe RedmineService, models: true do it { is_expected.not_to validate_presence_of(:new_issue_url) } end end + + describe '#reference_pattern' do + it_behaves_like 'allows project key on reference pattern' + + it 'does allow # on the reference' do + expect(subject.reference_pattern.match('#123')[:issue]).to eq('123') + end + end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 8dda34c7a03..ad5170afc21 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -415,7 +415,7 @@ describe GitPushService, services: true do it "doesn't close issues when external issue tracker is in use" do allow_any_instance_of(Project).to receive(:default_issues_tracker?). and_return(false) - external_issue_tracker = double(title: 'My Tracker', issue_path: issue.iid) + external_issue_tracker = double(title: 'My Tracker', issue_path: issue.iid, reference_pattern: project.issue_reference_pattern) allow_any_instance_of(Project).to receive(:external_issue_tracker).and_return(external_issue_tracker) # The push still shouldn't create cross-reference notes. @@ -484,30 +484,46 @@ describe GitPushService, services: true do end context "closing an issue" do - let(:message) { "this is some work.\n\ncloses JIRA-1" } - - it "initiates one api call to jira server to close the issue" do - transition_body = { - transition: { - id: '2' - } - }.to_json - - execute_service(project, commit_author, @oldrev, @newrev, @ref ) - expect(WebMock).to have_requested(:post, jira_api_transition_url).with( - body: transition_body - ).once + let(:message) { "this is some work.\n\ncloses JIRA-1" } + let(:transition_body) { { transition: { id: '2' } }.to_json } + let(:comment_body) { { body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json } + + context "using right markdown" do + it "initiates one api call to jira server to close the issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + + expect(WebMock).to have_requested(:post, jira_api_transition_url).with( + body: transition_body + ).once + end + + it "initiates one api call to jira server to comment on the issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + + expect(WebMock).to have_requested(:post, jira_api_comment_url).with( + body: comment_body + ).once + end end - it "initiates one api call to jira server to comment on the issue" do - comment_body = { - body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." - }.to_json + context "using wrong markdown" do + let(:message) { "this is some work.\n\ncloses #1" } - execute_service(project, commit_author, @oldrev, @newrev, @ref ) - expect(WebMock).to have_requested(:post, jira_api_comment_url).with( - body: comment_body - ).once + it "does not initiates one api call to jira server to close the issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + + expect(WebMock).not_to have_requested(:post, jira_api_transition_url).with( + body: transition_body + ) + end + + it "does not initiates one api call to jira server to comment on the issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + + expect(WebMock).not_to have_requested(:post, jira_api_comment_url).with( + body: comment_body + ).once + end end end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 9163c0c104e..f93d7732a9a 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -74,6 +74,18 @@ describe MergeRequests::MergeService, services: true do service.execute(merge_request) end + + context "wrong issue markdown" do + it 'does not close issues on JIRA issue tracker' do + jira_issue = ExternalIssue.new('#123', project) + commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") + allow(merge_request).to receive(:commits).and_return([commit]) + + expect_any_instance_of(JiraService).not_to receive(:close_issue) + + service.execute(merge_request) + end + end end end diff --git a/spec/support/issue_tracker_service_shared_example.rb b/spec/support/issue_tracker_service_shared_example.rb index b6d7436c360..e70b3963d9d 100644 --- a/spec/support/issue_tracker_service_shared_example.rb +++ b/spec/support/issue_tracker_service_shared_example.rb @@ -5,3 +5,18 @@ RSpec.shared_examples 'issue tracker service URL attribute' do |url_attr| it { is_expected.not_to allow_value('ftp://example.com').for(url_attr) } it { is_expected.not_to allow_value('herp-and-derp').for(url_attr) } end + +RSpec.shared_examples 'allows project key on reference pattern' do |url_attr| + it 'allows underscores in the project name' do + expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234' + end + + it 'allows numbers in the project name' do + expect(subject.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234' + end + + it 'requires the project name to begin with A-Z' do + expect(subject.reference_pattern.match('3EXT_EXT-1234')).to eq nil + expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234' + end +end |