diff options
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 9 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 4 | ||||
-rw-r--r-- | app/services/issues/move_service.rb | 87 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 24 | ||||
-rw-r--r-- | db/migrate/20160317092222_add_moved_to_to_issue.rb | 2 | ||||
-rw-r--r-- | db/schema.rb | 1 | ||||
-rw-r--r-- | lib/gitlab/gfm/reference_unfolder.rb | 25 | ||||
-rw-r--r-- | lib/gitlab/reference_extractor.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/reference_extractor_spec.rb | 5 | ||||
-rw-r--r-- | spec/services/issues/move_service_spec.rb | 72 |
11 files changed, 105 insertions, 142 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 9ec342a7b2a..016aa55656e 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -88,11 +88,11 @@ class Projects::IssuesController < Projects::ApplicationController def update @issue = Issues::UpdateService.new(project, current_user, issue_params).execute(issue) - move_service = Issues::MoveService.new(project, current_user, issue_params, - @issue, params['move_to_project_id']) - if move_service.move? - @issue = move_service.execute + if params[:move_to_project_id].to_i > 0 + new_project = Project.find(params[:move_to_project_id]) + move_service = Issues::MoveService.new(project, current_user, issue_params) + @issue = move_service.execute(@issue, new_project) end respond_to do |format| diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 86ab84615ba..9ab72652190 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -209,4 +209,13 @@ module Issuable Taskable.get_updated_tasks(old_content: previous_changes['description'].first, new_content: description) end + + ## + # Method that checks if issuable can be moved to another project. + # + # Should be overridden if issuable can be moved. + # + def can_move?(*) + false + end end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 8af1a9b8d3b..e63e1af8766 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -1,10 +1,10 @@ module Issues class CreateService < Issues::BaseService - def execute(set_author: true) + def execute filter_params label_params = params[:label_ids] issue = project.issues.new(params.except(:label_ids)) - issue.author = current_user if set_author + issue.author = params[:author] || current_user if issue.save issue.update_attributes(label_ids: label_params) diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index 47b58d973f9..45b4b50c0f6 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -1,23 +1,19 @@ module Issues class MoveService < Issues::BaseService - def initialize(project, current_user, params, issue, new_project_id) - super(project, current_user, params) + class MoveError < StandardError; end - @issue_old = issue - @issue_new = nil - @project_old = @project + def execute(issue, new_project) + @old_issue = issue + @old_project = @project + @new_project = new_project - if new_project_id.to_i > 0 - @project_new = Project.find(new_project_id) + unless issue.can_move?(current_user, new_project) + raise MoveError, 'Cannot move issue due to insufficient permissions!' end - if @project_new == @project_old - raise StandardError, 'Cannot move issue to project it originates from!' + if @project == new_project + raise MoveError, 'Cannot move issue to project it originates from!' end - end - - def execute - return unless move? # Using transaction because of a high resources footprint # on rewriting notes (unfolding references) @@ -25,81 +21,72 @@ module Issues ActiveRecord::Base.transaction do # New issue tasks # - create_new_issue + @new_issue = create_new_issue + rewrite_notes - add_moved_from_note + add_note_moved_from # Old issue tasks # - add_moved_to_note - close_old_issue + add_note_moved_to + close_issue mark_as_moved end notify_participants - @issue_new - end - - def move? - @project_new && can_move? + @new_issue end private - def can_move? - @issue_old.can_move?(@current_user) && - @issue_old.can_move?(@current_user, @project_new) - end - def create_new_issue - new_params = { id: nil, iid: nil, milestone: nil, label_ids: [], - project: @project_new, author: @issue_old.author, - description: unfold_references(@issue_old.description) } - - new_params = @issue_old.serializable_hash.merge(new_params) - create_service = CreateService.new(@project_new, @current_user, - new_params) + new_params = { id: nil, iid: nil, label_ids: [], milestone: nil, + project: @new_project, author: @old_issue.author, + description: unfold_references(@old_issue.description) } - @issue_new = create_service.execute(set_author: false) + new_params = @old_issue.serializable_hash.merge(new_params) + CreateService.new(@new_project, @current_user, new_params).execute end def rewrite_notes - @issue_old.notes.find_each do |note| + @old_issue.notes.find_each do |note| new_note = note.dup - new_params = { project: @project_new, noteable: @issue_new, + new_params = { project: @new_project, noteable: @new_issue, note: unfold_references(new_note.note) } new_note.update(new_params) end end - def close_old_issue - close_service = CloseService.new(@project_new, @current_user) - close_service.execute(@issue_old, notifications: false, system_note: false) + def close_issue + close_service = CloseService.new(@old_project, @current_user) + close_service.execute(@old_issue, notifications: false, system_note: false) end - def add_moved_from_note - SystemNoteService.noteable_moved(@issue_new, @project_new, - @issue_old, @current_user, direction: :from) + def add_note_moved_from + SystemNoteService.noteable_moved(@new_issue, @new_project, + @old_issue, @current_user, + direction: :from) end - def add_moved_to_note - SystemNoteService.noteable_moved(@issue_old, @project_old, - @issue_new, @current_user, direction: :to) + def add_note_moved_to + SystemNoteService.noteable_moved(@old_issue, @old_project, + @new_issue, @current_user, + direction: :to) end def unfold_references(content) - unfolder = Gitlab::Gfm::ReferenceUnfolder.new(content, @project_old) - unfolder.unfold(@project_new) + unfolder = Gitlab::Gfm::ReferenceUnfolder.new(content, @old_project) + unfolder.unfold(@new_project) end def notify_participants - notification_service.issue_moved(@issue_old, @issue_new, @current_user) + notification_service.issue_moved(@old_issue, @new_issue, @current_user) end def mark_as_moved - @issue_old.update(moved_to: @issue_new) + @old_issue.update(moved_to: @new_issue) end end end diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 5276afec1ca..d2fa407f8d3 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -67,22 +67,22 @@ - if can? current_user, :admin_label, issuable.project = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank -- if issuable.is_a?(Issue) && issuable.can_move?(current_user) +- if issuable.can_move?(current_user) %hr - .form-group - = label_tag :move_to_project_id, 'Move', class: 'control-label' - .col-sm-10 - - projects = project_options(issuable, current_user, ability: :admin_issue) - = select_tag(:move_to_project_id, projects, include_blank: true, - class: 'select2', data: { placeholder: 'Select project' }) + .form-group + = label_tag :move_to_project_id, 'Move', class: 'control-label' + .col-sm-10 + - projects = project_options(issuable, current_user, ability: :admin_issue) + = select_tag(:move_to_project_id, projects, include_blank: true, + class: 'select2', data: { placeholder: 'Select project' }) - if issuable.is_a?(MergeRequest) %hr - - if @merge_request.new_record? - .form-group - = f.label :source_branch, class: 'control-label' - .col-sm-10 - = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true }) + - if @merge_request.new_record? + .form-group + = f.label :source_branch, class: 'control-label' + .col-sm-10 + = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true }) .form-group = f.label :target_branch, class: 'control-label' .col-sm-10 diff --git a/db/migrate/20160317092222_add_moved_to_to_issue.rb b/db/migrate/20160317092222_add_moved_to_to_issue.rb index d262983b130..461e7fb3a9b 100644 --- a/db/migrate/20160317092222_add_moved_to_to_issue.rb +++ b/db/migrate/20160317092222_add_moved_to_to_issue.rb @@ -1,5 +1,5 @@ class AddMovedToToIssue < ActiveRecord::Migration def change - add_reference :issues, :moved_to, references: :issues, index: true + add_reference :issues, :moved_to, references: :issues end end diff --git a/db/schema.rb b/db/schema.rb index aa0071094df..968a1c92253 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -425,7 +425,6 @@ ActiveRecord::Schema.define(version: 20160317092222) do add_index "issues", ["created_at"], name: "index_issues_on_created_at", using: :btree add_index "issues", ["description"], name: "index_issues_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree - add_index "issues", ["moved_to_id"], name: "index_issues_on_moved_to_id", using: :btree add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree add_index "issues", ["project_id"], name: "index_issues_on_project_id", using: :btree add_index "issues", ["state"], name: "index_issues_on_state", using: :btree diff --git a/lib/gitlab/gfm/reference_unfolder.rb b/lib/gitlab/gfm/reference_unfolder.rb index 0a68d6f977f..cf8de88b856 100644 --- a/lib/gitlab/gfm/reference_unfolder.rb +++ b/lib/gitlab/gfm/reference_unfolder.rb @@ -7,7 +7,9 @@ module Gitlab # in context of. # # `unfold` method tries to find all local references and unfold each of - # those local references to cross reference format. + # those local references to cross reference format, assuming that the + # argument passed to this method is a project that references will be + # viewed from (see `Referable#to_reference method). # # Examples: # @@ -34,17 +36,12 @@ module Gitlab end def unfold(from_project) - return @text unless @text =~ references_pattern + pattern = Gitlab::ReferenceExtractor.references_pattern + return @text unless @text =~ pattern - unfolded = @text.gsub(references_pattern) do |reference| + @text.gsub(pattern) do |reference| unfold_reference(reference, Regexp.last_match, from_project) end - - unless substitution_valid?(unfolded) - raise StandardError, 'Invalid references unfolding!' - end - - unfolded end private @@ -61,16 +58,6 @@ module Gitlab substitution_valid?(new_text) ? cross_reference : reference end - def references_pattern - return @pattern if @pattern - - patterns = Gitlab::ReferenceExtractor::REFERABLES.map do |ref| - ref.to_s.classify.constantize.try(:reference_pattern) - end - - @pattern = Regexp.union(patterns.compact) - end - def referables return @referables if @referables diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb index 8c698d43bef..13c4d64c99b 100644 --- a/lib/gitlab/reference_extractor.rb +++ b/lib/gitlab/reference_extractor.rb @@ -37,6 +37,16 @@ module Gitlab @references.values.flatten end + def self.references_pattern + return @pattern if @pattern + + patterns = REFERABLES.map do |ref| + ref.to_s.classify.constantize.try(:reference_pattern) + end + + @pattern = Regexp.union(patterns.compact) + end + private def reference_context diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 78ab162e5d6..ba47a3540ff 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -134,4 +134,9 @@ describe Gitlab::ReferenceExtractor, lib: true do expect(subject.all).to match_array([issue, label]) end end + + describe '.references_pattern' do + subject { described_class.references_pattern } + it { is_expected.to be_kind_of Regexp } + end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 02ade91ac03..fd958a44e3e 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -15,11 +15,7 @@ describe Issues::MoveService, services: true do end let(:move_service) do - described_class.new(old_project, user, issue_params, old_issue, new_project_id) - end - - shared_context 'issue move requested' do - let(:new_project_id) { new_project.id } + described_class.new(old_project, user, issue_params) end shared_context 'user can move issue' do @@ -29,19 +25,13 @@ describe Issues::MoveService, services: true do end end - context 'issue movable' do - include_context 'issue move requested' - include_context 'user can move issue' - - describe '#move?' do - subject { move_service.move? } - it { is_expected.to be_truthy } + describe '#execute' do + shared_context 'issue move executed' do + let!(:new_issue) { move_service.execute(old_issue, new_project) } end - describe '#execute' do - shared_context 'issue move executed' do - let!(:new_issue) { move_service.execute } - end + context 'issue movable' do + include_context 'user can move issue' context 'generic issue' do include_context 'issue move executed' @@ -164,57 +154,33 @@ describe Issues::MoveService, services: true do end end end - end - end - - context 'moving to same project' do - let(:new_project) { old_project } - - include_context 'issue move requested' - include_context 'user can move issue' - - it 'raises error' do - expect { move_service } - .to raise_error(StandardError, /Cannot move issue/) - end - end - - context 'issue move not requested' do - let(:new_project_id) { nil } - describe '#move?' do - subject { move_service.move? } - - context 'user do not have permissions to move issue' do - it { is_expected.to be_falsey } - end + context 'moving to same project' do + let(:new_project) { old_project } - context 'user has permissions to move issue' do - include_context 'user can move issue' - it { is_expected.to be_falsey } + it 'raises error' do + expect { move_service.execute(old_issue, new_project) } + .to raise_error(StandardError, /Cannot move issue/) + end end end - end - - describe 'move permissions' do - include_context 'issue move requested' - describe '#move?' do - subject { move_service.move? } + describe 'move permissions' do + let(:move) { move_service.execute(old_issue, new_project) } context 'user is reporter in both projects' do include_context 'user can move issue' - it { is_expected.to be_truthy } + it { expect { move }.to_not raise_error } end context 'user is reporter only in new project' do before { new_project.team << [user, :reporter] } - it { is_expected.to be_falsey } + it { expect { move }.to raise_error(StandardError, /permissions/) } end context 'user is reporter only in old project' do before { old_project.team << [user, :reporter] } - it { is_expected.to be_falsey } + it { expect { move }.to raise_error(StandardError, /permissions/) } end context 'user is reporter in one project and guest in another' do @@ -223,7 +189,7 @@ describe Issues::MoveService, services: true do old_project.team << [user, :reporter] end - it { is_expected.to be_falsey } + it { expect { move }.to raise_error(StandardError, /permissions/) } end context 'issue has already been moved' do @@ -236,7 +202,7 @@ describe Issues::MoveService, services: true do moved_to: moved_to_issue) end - it { is_expected.to be_falsey } + it { expect { move }.to raise_error(StandardError, /permissions/) } end end end |