From 7bee7b848aab883a6869e1fd2fbb9e66182d2023 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Mon, 10 Jul 2017 09:38:42 +0200 Subject: Support both internal and external issue trackers --- app/controllers/projects/issues_controller.rb | 4 - app/helpers/issues_helper.rb | 26 ++- app/models/merge_request.rb | 2 +- app/models/project.rb | 10 +- .../project_services/issue_tracker_service.rb | 8 +- app/models/project_services/jira_service.rb | 2 +- app/services/issues/close_service.rb | 4 +- .../layouts/nav/_new_project_sidebar.html.haml | 6 +- app/views/layouts/nav/_project.html.haml | 6 +- app/views/projects/merge_requests/index.html.haml | 2 +- app/views/shared/_mr_head.html.haml | 2 +- lib/api/entities.rb | 2 +- lib/api/merge_requests.rb | 17 +- lib/banzai/filter/issue_reference_filter.rb | 2 +- .../reference_parser/external_issue_parser.rb | 10 +- lib/gitlab/reference_extractor.rb | 7 +- lib/gitlab/slash_commands/issue_command.rb | 2 +- .../features/issuables/markdown_references_spec.rb | 193 +++++++++++++++++++++ spec/features/projects/features_visibility_spec.rb | 2 +- spec/helpers/issues_helper_spec.rb | 8 +- .../filter/external_issue_reference_filter_spec.rb | 5 + spec/lib/banzai/pipeline/gfm_pipeline_spec.rb | 89 ++++++++-- spec/lib/gitlab/reference_extractor_spec.rb | 31 +++- spec/models/concerns/mentionable_spec.rb | 18 +- spec/models/merge_request_spec.rb | 52 +++++- spec/models/project_spec.rb | 45 ++++- spec/requests/api/merge_requests_spec.rb | 14 +- spec/requests/api/projects_spec.rb | 25 +++ spec/services/git_push_service_spec.rb | 58 +++++-- spec/services/issues/close_service_spec.rb | 8 +- spec/services/merge_requests/build_service_spec.rb | 2 +- 31 files changed, 563 insertions(+), 99 deletions(-) create mode 100644 spec/features/issuables/markdown_references_spec.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 5b0eeac2477..e2ccabb22db 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -269,10 +269,6 @@ class Projects::IssuesController < Projects::ApplicationController end end - def module_enabled - render_404 unless @project.feature_available?(:issues, current_user) - end - def issue_params params.require(:issue).permit(*issue_params_attributes) end diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 42b6cfdf02f..7e1ccb23e9e 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -17,10 +17,10 @@ module IssuesHelper return '' if project.nil? url = - if options[:only_path] - project.issues_tracker.issue_path(issue_iid) + if options[:internal] + url_for_internal_issue(issue_iid, project, options) else - project.issues_tracker.issue_url(issue_iid) + url_for_tracker_issue(issue_iid, project, options) end # Ensure we return a valid URL to prevent possible XSS. @@ -29,6 +29,24 @@ module IssuesHelper '' end + def url_for_tracker_issue(issue_iid, project, options) + if options[:only_path] + project.issues_tracker.issue_path(issue_iid) + else + project.issues_tracker.issue_url(issue_iid) + end + end + + def url_for_internal_issue(issue_iid, project = @project, options = {}) + helpers = Gitlab::Routing.url_helpers + + if options[:only_path] + helpers.namespace_project_issue_path(namespace_id: project.namespace, project_id: project, id: issue_iid) + else + helpers.namespace_project_issue_url(namespace_id: project.namespace, project_id: project, id: issue_iid) + end + end + def bulk_update_milestone_options milestones = @project.milestones.active.reorder(due_date: :asc, title: :asc).to_a milestones.unshift(Milestone::None) @@ -158,4 +176,6 @@ module IssuesHelper # Required for Banzai::Filter::IssueReferenceFilter module_function :url_for_issue + module_function :url_for_internal_issue + module_function :url_for_tracker_issue end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e4e7999d0f2..a910099b4c1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -596,7 +596,7 @@ class MergeRequest < ActiveRecord::Base # running `ReferenceExtractor` on each of them separately. # This optimization does not apply to issues from external sources. def cache_merge_request_closes_issues!(current_user) - return if project.has_external_issue_tracker? + return unless project.issues_enabled? transaction do self.merge_requests_closing_issues.delete_all diff --git a/app/models/project.rb b/app/models/project.rb index 0b357d5d003..d827bfaa806 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -734,9 +734,11 @@ class Project < ActiveRecord::Base end def get_issue(issue_id, current_user) - if default_issues_tracker? - IssuesFinder.new(current_user, project_id: id).find_by(iid: issue_id) - else + issue = IssuesFinder.new(current_user, project_id: id).find_by(iid: issue_id) if issues_enabled? + + if issue + issue + elsif external_issue_tracker ExternalIssue.new(issue_id, self) end end @@ -758,7 +760,7 @@ class Project < ActiveRecord::Base end def external_issue_reference_pattern - external_issue_tracker.class.reference_pattern + external_issue_tracker.class.reference_pattern(only_long: issues_enabled?) end def default_issues_tracker? diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index 6d6a3ae3647..31984c5d7ed 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -8,8 +8,12 @@ class IssueTrackerService < Service # This pattern does not support cross-project references # The other code assumes that this pattern is a superset of all # overriden patterns. See ReferenceRegexes::EXTERNAL_PATTERN - def self.reference_pattern - @reference_pattern ||= %r{(\b[A-Z][A-Z0-9_]+-|#{Issue.reference_prefix})(?\d+)} + def self.reference_pattern(only_long: false) + if only_long + %r{(\b[A-Z][A-Z0-9_]+-)(?\d+)} + else + %r{(\b[A-Z][A-Z0-9_]+-|#{Issue.reference_prefix})(?\d+)} + end end def default? diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 5498a2e17b2..450027c2e57 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -18,7 +18,7 @@ class JiraService < IssueTrackerService end # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1 - def self.reference_pattern + def self.reference_pattern(only_long: true) @reference_pattern ||= %r{(?\b([A-Z][A-Z0-9_]+-)\d+)} end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index ddef5281498..74459c3342c 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -16,13 +16,13 @@ module Issues # The code calling this method is responsible for ensuring that a user is # allowed to close the given issue. def close_issue(issue, commit: nil, notifications: true, system_note: true) - if project.jira_tracker? && project.jira_service.active + if project.jira_tracker? && project.jira_service.active && issue.is_a?(ExternalIssue) project.jira_service.close_issue(commit, issue) todo_service.close_issue(issue, current_user) return issue end - if project.default_issues_tracker? && issue.close + if project.issues_enabled? && issue.close event_service.close_issue(issue, current_user) create_note(issue, commit) if system_note notification_service.close_issue(issue, current_user) if notifications diff --git a/app/views/layouts/nav/_new_project_sidebar.html.haml b/app/views/layouts/nav/_new_project_sidebar.html.haml index 21f175291fa..00395b222e4 100644 --- a/app/views/layouts/nav/_new_project_sidebar.html.haml +++ b/app/views/layouts/nav/_new_project_sidebar.html.haml @@ -75,10 +75,10 @@ Registry - if project_nav_tab? :issues - = nav_link(controller: @project.default_issues_tracker? ? [:issues, :labels, :milestones, :boards] : :issues) do + = nav_link(controller: @project.issues_enabled? ? [:issues, :labels, :milestones, :boards] : :issues) do = link_to project_issues_path(@project), title: 'Issues', class: 'shortcuts-issues' do %span - - if @project.default_issues_tracker? + - if @project.issues_enabled? %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count) Issues @@ -113,7 +113,7 @@ Milestones - if project_nav_tab? :merge_requests - = nav_link(controller: @project.default_issues_tracker? ? :merge_requests : [:merge_requests, :labels, :milestones]) do + = nav_link(controller: @project.issues_enabled? ? :merge_requests : [:merge_requests, :labels, :milestones]) do = link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do %span %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count) diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index fb90bb4b472..924cd2e9681 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -23,16 +23,16 @@ Registry - if project_nav_tab? :issues - = nav_link(controller: @project.default_issues_tracker? ? [:issues, :labels, :milestones, :boards] : :issues) do + = nav_link(controller: @project.issues_enabled? ? [:issues, :labels, :milestones, :boards] : :issues) do = link_to project_issues_path(@project), title: 'Issues', class: 'shortcuts-issues' do %span Issues - - if @project.default_issues_tracker? + - if @project.issues_enabled? %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id))) - if project_nav_tab? :merge_requests - controllers = [:merge_requests, 'projects/merge_requests/conflicts'] - - controllers.push(:merge_requests, :labels, :milestones) unless @project.default_issues_tracker? + - controllers.push(:merge_requests, :labels, :milestones) unless @project.issues_enabled? = nav_link(controller: controllers) do = link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do %span diff --git a/app/views/projects/merge_requests/index.html.haml b/app/views/projects/merge_requests/index.html.haml index bfeb746ee83..c020e7db380 100644 --- a/app/views/projects/merge_requests/index.html.haml +++ b/app/views/projects/merge_requests/index.html.haml @@ -4,7 +4,7 @@ - new_merge_request_path = project_new_merge_request_path(merge_project) if merge_project - page_title "Merge Requests" -- unless @project.default_issues_tracker? +- unless @project.issues_enabled? = content_for :sub_nav do = render "projects/merge_requests/head" diff --git a/app/views/shared/_mr_head.html.haml b/app/views/shared/_mr_head.html.haml index 4211ec6351d..e7355ae2eea 100644 --- a/app/views/shared/_mr_head.html.haml +++ b/app/views/shared/_mr_head.html.haml @@ -1,4 +1,4 @@ -- if @project.default_issues_tracker? +- if @project.issues_enabled? = render "projects/issues/head" - else = render "projects/merge_requests/head" diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 09a88869063..1719e9f7205 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -109,7 +109,7 @@ module API user.avatar_url(only_path: false) end expose :star_count, :forks_count - expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? } + expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] } expose :public_builds, as: :public_jobs expose :ci_config_path diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 6e2e13e0a24..f64ac659413 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -29,14 +29,6 @@ module API render_api_error!(errors, 400) end - def issue_entity(project) - if project.has_external_issue_tracker? - Entities::ExternalIssue - else - Entities::IssueBasic - end - end - def find_merge_requests(args = {}) args = params.merge(args) @@ -278,7 +270,14 @@ module API get ':id/merge_requests/:merge_request_iid/closes_issues' do merge_request = find_merge_request_with_access(params[:merge_request_iid]) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) - present paginate(issues), with: issue_entity(user_project), current_user: current_user + issues = paginate(issues) + + external_issues, internal_issues = issues.partition { |issue| issue.is_a?(ExternalIssue) } + + data = Entities::IssueBasic.represent(internal_issues, current_user: current_user) + data += Entities::ExternalIssue.represent(external_issues, current_user: current_user) + + data.as_json end end end diff --git a/lib/banzai/filter/issue_reference_filter.rb b/lib/banzai/filter/issue_reference_filter.rb index ba1a5ac84b3..ce1ab977d3b 100644 --- a/lib/banzai/filter/issue_reference_filter.rb +++ b/lib/banzai/filter/issue_reference_filter.rb @@ -20,7 +20,7 @@ module Banzai end def url_for_object(issue, project) - IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path]) + IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path], internal: true) end def project_from_ref(ref) diff --git a/lib/banzai/reference_parser/external_issue_parser.rb b/lib/banzai/reference_parser/external_issue_parser.rb index 6307c1b571a..1802cd04854 100644 --- a/lib/banzai/reference_parser/external_issue_parser.rb +++ b/lib/banzai/reference_parser/external_issue_parser.rb @@ -21,10 +21,14 @@ module Banzai gather_attributes_per_project(nodes, self.class.data_attribute) end - private - + # we extract only external issue trackers references here, we don't extract cross-project references, + # so we don't need to do anything here. def can_read_reference?(user, ref_project, node) - can?(user, :read_issue, ref_project) + true + end + + def nodes_visible_to_user(user, nodes) + nodes end end end diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 7668ecacc4b..f5b757ace77 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -33,7 +33,12 @@ module Gitlab def issues if project && project.jira_tracker? - @references[:external_issue] ||= references(:external_issue) + if project.issues_enabled? + @references[:all_issues] ||= references(:external_issue) + references(:issue) + else + @references[:external_issue] ||= references(:external_issue) + + references(:issue).select { |i| i.project_id != project.id } + end else @references[:issue] ||= references(:issue) end diff --git a/lib/gitlab/slash_commands/issue_command.rb b/lib/gitlab/slash_commands/issue_command.rb index 87ea19b8806..3d96982b820 100644 --- a/lib/gitlab/slash_commands/issue_command.rb +++ b/lib/gitlab/slash_commands/issue_command.rb @@ -2,7 +2,7 @@ module Gitlab module SlashCommands class IssueCommand < BaseCommand def self.available?(project) - project.issues_enabled? && project.default_issues_tracker? + project.issues_enabled? end def collection diff --git a/spec/features/issuables/markdown_references_spec.rb b/spec/features/issuables/markdown_references_spec.rb new file mode 100644 index 00000000000..f51b2e4001a --- /dev/null +++ b/spec/features/issuables/markdown_references_spec.rb @@ -0,0 +1,193 @@ +require 'rails_helper' + +describe 'Markdown References', :feature, :js do + let(:user) { create(:user) } + let(:actual_project) { create(:project, :public) } + let(:merge_request) { create(:merge_request, target_project: actual_project, source_project: actual_project)} + let(:issue_actual_project) { create(:issue, project: actual_project) } + let!(:other_project) { create(:empty_project, :public) } + let!(:issue_other_project) { create(:issue, project: other_project) } + let(:issues) { [issue_actual_project, issue_other_project] } + + def build_note + markdown = "Referencing internal issue #{issue_actual_project.to_reference}, " + + "cross-project #{issue_other_project.to_reference(actual_project)} external JIRA-5 " + + "and non existing #999" + + page.within('#diff-notes-app') do + fill_in 'note_note', with: markdown + end + end + + shared_examples 'correct references' do + before do + remotelink = double(:remotelink, all: [], build: double(save!: true)) + + stub_request(:get, "https://jira.example.com/rest/api/2/issue/JIRA-5") + stub_request(:post, "https://jira.example.com/rest/api/2/issue/JIRA-5/comment") + allow_any_instance_of(JIRA::Resource::Issue).to receive(:remotelink).and_return(remotelink) + + sign_in(user) + visit merge_request_path(merge_request) + build_note + end + + def links_expectations + issues.each do |issue| + if referenced_issues.include?(issue) + expect(page).to have_link(issue.to_reference, href: issue_path(issue)) + else + expect(page).not_to have_link(issue.to_reference, href: issue_path(issue)) + end + end + + if jira_referenced + expect(page).to have_link('JIRA-5', href: 'https://jira.example.com/browse/JIRA-5') + else + expect(page).not_to have_link('JIRA-5', href: 'https://jira.example.com/browse/JIRA-5') + end + + expect(page).not_to have_link('#999') + end + + it 'creates a link to the referenced issue on the preview' do + find('.js-md-preview-button').click + wait_for_requests + + page.within('.md-preview-holder') do + links_expectations + end + end + + it 'creates a link to the referenced issue after submit' do + click_button 'Comment' + wait_for_requests + + page.within('#diff-notes-app') do + links_expectations + end + end + + it 'creates a note on the referenced issues' do + click_button 'Comment' + wait_for_requests + + if referenced_issues.include?(issue_actual_project) + visit issue_path(issue_actual_project) + + page.within('#notes') do + expect(page).to have_content( + "#{user.to_reference} mentioned in merge request #{merge_request.to_reference}" + ) + end + end + + if referenced_issues.include?(issue_other_project) + visit issue_path(issue_other_project) + + page.within('#notes') do + expect(page).to have_content( + "#{user.to_reference} mentioned in merge request #{merge_request.to_reference(other_project)}" + ) + end + end + end + end + + context 'when internal issues tracker is enabled for the other project' do + context 'when only internal issues tracker is enabled for the actual project' do + include_examples 'correct references' do + let(:referenced_issues) { [issue_actual_project, issue_other_project] } + let(:jira_referenced) { false } + end + end + + context 'when both external and internal issues trackers are enabled for the actual project' do + before do + create(:jira_service, project: actual_project) + end + + include_examples 'correct references' do + let(:referenced_issues) { [issue_actual_project, issue_other_project] } + let(:jira_referenced) { true } + end + end + + context 'when only external issues tracker is enabled for the actual project' do + before do + create(:jira_service, project: actual_project) + + actual_project.issues_enabled = false + actual_project.save! + end + + include_examples 'correct references' do + let(:referenced_issues) { [issue_other_project] } + let(:jira_referenced) { true } + end + end + + context 'when no tracker is enabled for the actual project' do + before do + actual_project.issues_enabled = false + actual_project.save! + end + + include_examples 'correct references' do + let(:referenced_issues) { [issue_other_project] } + let(:jira_referenced) { false } + end + end + end + + context 'when internal issues tracker is disabled for the other project' do + before do + other_project.issues_enabled = false + other_project.save! + end + + context 'when only internal issues tracker is enabled for the actual project' do + include_examples 'correct references' do + let(:referenced_issues) { [issue_actual_project] } + let(:jira_referenced) { false } + end + end + + context 'when both external and internal issues trackers are enabled for the actual project' do + before do + create(:jira_service, project: actual_project) + end + + include_examples 'correct references' do + let(:referenced_issues) { [issue_actual_project] } + let(:jira_referenced) { true } + end + end + + context 'when only external issues tracker is enabled for the actual project' do + before do + create(:jira_service, project: actual_project) + + actual_project.issues_enabled = false + actual_project.save! + end + + include_examples 'correct references' do + let(:referenced_issues) { [] } + let(:jira_referenced) { true } + end + end + + context 'when no issues tracker is enabled for the actual project' do + before do + actual_project.issues_enabled = false + actual_project.save! + end + + include_examples 'correct references' do + let(:referenced_issues) { [] } + let(:jira_referenced) { false } + end + end + end +end diff --git a/spec/features/projects/features_visibility_spec.rb b/spec/features/projects/features_visibility_spec.rb index 2091c7b79d3..1588f8a828a 100644 --- a/spec/features/projects/features_visibility_spec.rb +++ b/spec/features/projects/features_visibility_spec.rb @@ -55,7 +55,7 @@ describe 'Edit Project Settings', feature: true do project.save! allow_any_instance_of(Project).to receive(:external_issue_tracker).and_return(JiraService.new) - visit namespace_project_path(project.namespace, project) + visit project_path(project) expect(page).not_to have_selector('.shortcuts-issues') end diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index 8f7f17a484f..9524a101e74 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -8,7 +8,7 @@ describe IssuesHelper do describe "url_for_issue" do let(:issues_url) { ext_project.external_issue_tracker.issues_url} let(:ext_expected) { issues_url.gsub(':id', issue.iid.to_s).gsub(':project_id', ext_project.id.to_s) } - let(:int_expected) { polymorphic_path([@project.namespace, project, issue]) } + let(:int_expected) { polymorphic_path([@project.namespace, @project, issue]) } it "returns internal path if used internal tracker" do @project = project @@ -22,6 +22,12 @@ describe IssuesHelper do expect(url_for_issue(issue.iid)).to match(ext_expected) end + it "returns path to internal issue when internal option passed" do + @project = ext_project + + expect(url_for_issue(issue.iid, ext_project, internal: true)).to match(int_expected) + end + it "returns empty string if project nil" do @project = nil 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 b7d82c36ddd..fb320e0148a 100644 --- a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb @@ -108,6 +108,11 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do let(:issue) { ExternalIssue.new("#123", project) } let(:reference) { issue.to_reference } + before do + project.issues_enabled = false + project.save! + end + it_behaves_like "external issue tracker" end diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index 1eb90dc1847..601ffbb5456 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -4,26 +4,87 @@ describe Banzai::Pipeline::GfmPipeline do describe 'integration between parsing regular and external issue references' do let(:project) { create(:redmine_project, :public) } - it 'allows to use shorthand external reference syntax for Redmine' do - markdown = '#12' + context 'when internal issue tracker is enabled' do + context 'when shorthand pattern #ISSUE_ID is used' do + it 'links an internal issue if it exists' do + issue = create(:issue, project: project) + markdown = issue.to_reference(project, full: true) - result = described_class.call(markdown, project: project)[:output] - link = result.css('a').first + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first - expect(link['href']).to eq 'http://redmine/projects/project_name_in_redmine/issues/12' + expect(link['href']).to eq( + Gitlab::Routing.url_helpers.project_issue_path(project, issue) + ) + end + + it 'does not link any issue if it does not exist on GitLab' do + markdown = '#12' + + result = described_class.call(markdown, project: project)[:output] + expect(result.css('a')).to be_empty + end + end + + it 'allows to use long external reference syntax for Redmine' do + markdown = 'API_32-12' + + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first + + expect(link['href']).to eq 'http://redmine/projects/project_name_in_redmine/issues/12' + end + + it 'parses cross-project references to regular issues' do + other_project = create(:empty_project, :public) + issue = create(:issue, project: other_project) + markdown = issue.to_reference(project, full: true) + + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first + + expect(link['href']).to eq( + Gitlab::Routing.url_helpers.project_issue_path(other_project, issue) + ) + end end - it 'parses cross-project references to regular issues' do - other_project = create(:empty_project, :public) - issue = create(:issue, project: other_project) - markdown = issue.to_reference(project, full: true) + context 'when internal issue tracker is disabled' do + before do + project.issues_enabled = false + project.save! + end + + it 'allows to use shorthand external reference syntax for Redmine' do + markdown = '#12' + + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first + + expect(link['href']).to eq 'http://redmine/projects/project_name_in_redmine/issues/12' + end + + it 'allows to use long external reference syntax for Redmine' do + markdown = 'API_32-12' + + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first + + expect(link['href']).to eq 'http://redmine/projects/project_name_in_redmine/issues/12' + end + + it 'parses cross-project references to regular issues' do + other_project = create(:empty_project, :public) + issue = create(:issue, project: other_project) + markdown = issue.to_reference(project, full: true) - result = described_class.call(markdown, project: project)[:output] - link = result.css('a').first + result = described_class.call(markdown, project: project)[:output] + link = result.css('a').first - expect(link['href']).to eq( - Gitlab::Routing.url_helpers.project_issue_path(other_project, issue) - ) + expect(link['href']).to eq( + Gitlab::Routing.url_helpers.project_issue_path(other_project, issue) + ) + end end end end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 84cfd934fa0..917692e9c6c 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -183,11 +183,34 @@ describe Gitlab::ReferenceExtractor, lib: true do context 'with an external issue tracker' do let(:project) { create(:jira_project) } + let(:issue) { create(:issue, project: project) } + + context 'when GitLab issues are enabled' do + it 'returns both JIRA and internal issues' do + subject.analyze("JIRA-123 and FOOBAR-4567 and #{issue.to_reference}") + expect(subject.issues).to eq [ExternalIssue.new('JIRA-123', project), + ExternalIssue.new('FOOBAR-4567', project), + issue] + end + + it 'returns only JIRA issues if the internal one does not exists' do + subject.analyze("JIRA-123 and FOOBAR-4567 and #999") + expect(subject.issues).to eq [ExternalIssue.new('JIRA-123', project), + ExternalIssue.new('FOOBAR-4567', project)] + end + end - it 'returns JIRA issues for a JIRA-integrated project' do - subject.analyze('JIRA-123 and FOOBAR-4567') - expect(subject.issues).to eq [ExternalIssue.new('JIRA-123', project), - ExternalIssue.new('FOOBAR-4567', project)] + context 'when GitLab issues are disabled' do + before do + project.issues_enabled = false + project.save! + end + + it 'returns only JIRA issues' do + subject.analyze("JIRA-123 and FOOBAR-4567 and #{issue.to_reference}") + expect(subject.issues).to eq [ExternalIssue.new('JIRA-123', project), + ExternalIssue.new('FOOBAR-4567', project)] + end end end diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index e2a29e0ae70..1ad811736af 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -174,25 +174,25 @@ describe Commit, 'Mentionable' do it "is false when message doesn't reference anything" do allow(commit.raw).to receive(:message).and_return "WIP: Do something" - expect(commit.matches_cross_reference_regex?).to be false + expect(commit.matches_cross_reference_regex?).to be_falsey end it 'is true if issue #number mentioned in title' do allow(commit.raw).to receive(:message).and_return "#1" - expect(commit.matches_cross_reference_regex?).to be true + expect(commit.matches_cross_reference_regex?).to be_truthy end it 'is true if references an MR' do allow(commit.raw).to receive(:message).and_return "See merge request !12" - expect(commit.matches_cross_reference_regex?).to be true + expect(commit.matches_cross_reference_regex?).to be_truthy end it 'is true if references a commit' do allow(commit.raw).to receive(:message).and_return "a1b2c3d4" - expect(commit.matches_cross_reference_regex?).to be true + expect(commit.matches_cross_reference_regex?).to be_truthy end it 'is true if issue referenced by url' do @@ -200,7 +200,7 @@ describe Commit, 'Mentionable' do allow(commit.raw).to receive(:message).and_return Gitlab::UrlBuilder.build(issue) - expect(commit.matches_cross_reference_regex?).to be true + expect(commit.matches_cross_reference_regex?).to be_truthy end context 'with external issue tracker' do @@ -209,7 +209,13 @@ describe Commit, 'Mentionable' do it 'is true if external issues referenced' do allow(commit.raw).to receive(:message).and_return 'JIRA-123' - expect(commit.matches_cross_reference_regex?).to be true + expect(commit.matches_cross_reference_regex?).to be_truthy + end + + it 'is true if internal issues referenced' do + allow(commit.raw).to receive(:message).and_return '#123' + + expect(commit.matches_cross_reference_regex?).to be_truthy end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1eadc28869f..6f6a8ac91b8 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -155,13 +155,53 @@ describe MergeRequest, models: true do expect { subject.cache_merge_request_closes_issues!(subject.author) }.to change(subject.merge_requests_closing_issues, :count).by(1) end - it 'does not cache issues from external trackers' do - subject.project.update_attribute(:has_external_issue_tracker, true) - issue = ExternalIssue.new('JIRA-123', subject.project) - commit = double('commit1', safe_message: "Fixes #{issue.to_reference}") - allow(subject).to receive(:commits).and_return([commit]) + context 'when both internal and external issue trackers are enabled' do + before do + subject.project.has_external_issue_tracker = true + subject.project.save! + end + + it 'does not cache issues from external trackers' do + issue = ExternalIssue.new('JIRA-123', subject.project) + commit = double('commit1', safe_message: "Fixes #{issue.to_reference}") + allow(subject).to receive(:commits).and_return([commit]) - expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count) + expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count) + end + + it 'caches an internal issue' do + issue = create(:issue, project: subject.project) + commit = double('commit1', safe_message: "Fixes #{issue.to_reference}") + allow(subject).to receive(:commits).and_return([commit]) + + expect { subject.cache_merge_request_closes_issues!(subject.author) } + .to change(subject.merge_requests_closing_issues, :count).by(1) + end + end + + context 'when only external issue tracker enabled' do + before do + subject.project.has_external_issue_tracker = true + subject.project.issues_enabled = false + subject.project.save! + end + + it 'does not cache issues from external trackers' do + issue = ExternalIssue.new('JIRA-123', subject.project) + commit = double('commit1', safe_message: "Fixes #{issue.to_reference}") + allow(subject).to receive(:commits).and_return([commit]) + + expect { subject.cache_merge_request_closes_issues!(subject.author) }.not_to change(subject.merge_requests_closing_issues, :count) + end + + it 'does not cache an internal issue' do + issue = create(:issue, project: subject.project) + commit = double('commit1', safe_message: "Fixes #{issue.to_reference}") + allow(subject).to receive(:commits).and_return([commit]) + + expect { subject.cache_merge_request_closes_issues!(subject.author) } + .not_to change(subject.merge_requests_closing_issues, :count) + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index fdcb011d685..8d916b79b13 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -533,15 +533,48 @@ describe Project, models: true do end context 'with external issues tracker' do + let!(:internal_issue) { create(:issue, project: project) } before do - allow(project).to receive(:default_issues_tracker?).and_return(false) + allow(project).to receive(:external_issue_tracker).and_return(true) end - it 'returns an ExternalIssue' do - issue = project.get_issue('FOO-1234', user) - expect(issue).to be_kind_of(ExternalIssue) - expect(issue.iid).to eq 'FOO-1234' - expect(issue.project).to eq project + context 'when internal issues are enabled' do + it 'returns interlan issue' do + issue = project.get_issue(internal_issue.iid, user) + + expect(issue).to be_kind_of(Issue) + expect(issue.iid).to eq(internal_issue.iid) + expect(issue.project).to eq(project) + end + + it 'returns an ExternalIssue when internal issue does not exists' do + issue = project.get_issue('FOO-1234', user) + + expect(issue).to be_kind_of(ExternalIssue) + expect(issue.iid).to eq('FOO-1234') + expect(issue.project).to eq(project) + end + end + + context 'when internal issues are disabled' do + before do + project.issues_enabled = false + project.save! + end + + it 'returns always an External issues' do + issue = project.get_issue(internal_issue.iid, user) + expect(issue).to be_kind_of(ExternalIssue) + expect(issue.iid).to eq(internal_issue.iid.to_s) + expect(issue.project).to eq(project) + end + + it 'returns an ExternalIssue when internal issue does not exists' do + issue = project.get_issue('FOO-1234', user) + expect(issue).to be_kind_of(ExternalIssue) + expect(issue.iid).to eq('FOO-1234') + expect(issue.project).to eq(project) + end end end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 9098ae6bcda..35b6522ea98 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -794,18 +794,24 @@ describe API::MergeRequests do it 'handles external issues' do jira_project = create(:jira_project, :public, name: 'JIR_EXT1') - issue = ExternalIssue.new("#{jira_project.name}-123", jira_project) - merge_request = create(:merge_request, :simple, author: user, assignee: user, source_project: jira_project) - merge_request.update_attribute(:description, "Closes #{issue.to_reference(jira_project)}") + ext_issue = ExternalIssue.new("#{jira_project.name}-123", jira_project) + issue = create(:issue, project: jira_project) + description = "Closes #{ext_issue.to_reference(jira_project)}\ncloses #{issue.to_reference}" + merge_request = create(:merge_request, + :simple, author: user, assignee: user, source_project: jira_project, description: description) get api("/projects/#{jira_project.id}/merge_requests/#{merge_request.iid}/closes_issues", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(json_response.length).to eq(1) + expect(json_response.length).to eq(2) + expect(json_response.second['title']).to eq(ext_issue.title) + expect(json_response.second['id']).to eq(ext_issue.id) + expect(json_response.second['confidential']).to be_nil expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['id']).to eq(issue.id) + expect(json_response.first['confidential']).not_to be_nil end it 'returns 403 if the user has no access to the merge request' do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 6dbde8bad31..457f64cc88c 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -159,6 +159,31 @@ describe API::Projects do expect(json_response.first).to include 'statistics' end + context 'when external issue tracker is enabled' do + let!(:jira_service) { create(:jira_service, project: project) } + + it 'includes open_issues_count' do + get api('/projects', user) + + expect(response.status).to eq 200 + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.first.keys).to include('open_issues_count') + expect(json_response.find { |hash| hash['id'] == project.id }.keys).to include('open_issues_count') + end + + it 'does not include open_issues_count if issues are disabled' do + project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED) + + get api('/projects', user) + + expect(response.status).to eq 200 + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.find { |hash| hash['id'] == project.id }.keys).not_to include('open_issues_count') + end + end + context 'and with simple=true' do it 'returns a simplified version of all the projects' do expected_keys = %w(id http_url_to_repo web_url name name_with_namespace path path_with_namespace) diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index c493c08a7ae..f801506f1b6 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -488,21 +488,57 @@ describe GitPushService, services: true do end end - context "using wrong markdown" do - let(:message) { "this is some work.\n\ncloses #1" } + context "using internal issue reference" do + context 'when internal issues are disabled' do + before do + project.issues_enabled = false + project.save! + end + let(:message) { "this is some work.\n\ncloses #1" } + + 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('JIRA-1')) + 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('JIRA-1')).with( + body: comment_body + ).once + end + end - it "does not initiates one api call to jira server to close the issue" do - execute_service(project, commit_author, @oldrev, @newrev, @ref ) + context 'when internal issues are enabled' do + let(:issue) { create(:issue, project: project) } + let(:message) { "this is some work.\n\ncloses JIRA-1 \n\n closes #{issue.to_reference}" } - expect(WebMock).not_to have_requested(:post, jira_api_transition_url('JIRA-1')) - end + it "initiates one api call to jira server to close the jira issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) - 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).to have_requested(:post, jira_api_transition_url('JIRA-1')).once + end - expect(WebMock).not_to have_requested(:post, jira_api_comment_url('JIRA-1')).with( - body: comment_body - ).once + it "initiates one api call to jira server to comment on the jira issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + + expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( + body: comment_body + ).once + end + + it "closes the internal issue" do + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + expect(issue.reload).to be_closed + end + + it "adds a note indicating that the issue is now closed" do + expect(SystemNoteService).to receive(:change_status) + .with(issue, project, commit_author, "closed", closing_commit) + execute_service(project, commit_author, @oldrev, @newrev, @ref ) + end end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index d6f4c694069..da8b60f1337 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -98,13 +98,13 @@ describe Issues::CloseService, services: true do end end - context 'external issue tracker' do + context 'internal issues disabled' do before do - allow(project).to receive(:default_issues_tracker?).and_return(false) - described_class.new(project, user).close_issue(issue) + project.issues_enabled = false + project.save! end - it 'closes the issue' do + it 'does not close the issue' do expect(issue).to be_valid expect(issue).to be_opened expect(todo.reload).to be_pending diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 01ef52396d7..a40d4c877bc 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -207,7 +207,7 @@ describe MergeRequests::BuildService, services: true do let(:source_branch) { '12345-fix-issue' } before do - allow(project).to receive(:default_issues_tracker?).and_return(false) + allow(project).to receive(:external_issue_tracker).and_return(true) end it 'sets the title to: Resolves External Issue $issue-iid' do -- cgit v1.2.1