From b9036ba61012e6723426f98171a2c111abb0bae5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 17 Mar 2016 11:11:22 +0100 Subject: [PATCH] Prevent issue move if issue has been already moved --- app/models/issue.rb | 12 ++++++ app/services/issues/move_service.rb | 9 +++- app/views/shared/issuable/_form.html.haml | 2 +- spec/features/issues/move_spec.rb | 11 +++++ spec/models/issue_spec.rb | 50 +++++++++++++++++++++++ spec/services/issues/move_service_spec.rb | 18 ++++++++ 6 files changed, 99 insertions(+), 3 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index b506d174155..6a016636e0d 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -123,4 +123,16 @@ class Issue < ActiveRecord::Base note.all_references(current_user).merge_requests end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) } end + + def moved? + !moved_to.nil? + end + + def can_move?(user, to_project = nil) + if to_project + return false unless user.can?(:admin_issue, to_project) + end + + !moved? && user.can?(:admin_issue, self.project) + end end diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index ce31830f2d0..47b58d973f9 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -33,6 +33,7 @@ module Issues # add_moved_to_note close_old_issue + mark_as_moved end notify_participants @@ -47,8 +48,8 @@ module Issues private def can_move? - can?(@current_user, :admin_issue, @project_old) && - can?(@current_user, :admin_issue, @project_new) + @issue_old.can_move?(@current_user) && + @issue_old.can_move?(@current_user, @project_new) end def create_new_issue @@ -96,5 +97,9 @@ module Issues def notify_participants notification_service.issue_moved(@issue_old, @issue_new, @current_user) end + + def mark_as_moved + @issue_old.update(moved_to: @issue_new) + end end end diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index d3eabe9ea64..5276afec1ca 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -67,7 +67,7 @@ - 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) && can?(current_user, :admin_issue, issuable.project) +- if issuable.is_a?(Issue) && issuable.can_move?(current_user) %hr .form-group = label_tag :move_to_project_id, 'Move', class: 'control-label' diff --git a/spec/features/issues/move_spec.rb b/spec/features/issues/move_spec.rb index 6303b117c09..db26b3a4671 100644 --- a/spec/features/issues/move_spec.rb +++ b/spec/features/issues/move_spec.rb @@ -60,6 +60,17 @@ feature 'issue move to another project' do expect(page).to have_select('move_to_project_id', options: options) end end + + context 'issue has been already moved' do + let(:new_issue) { create(:issue, project: new_project) } + let(:issue) do + create(:issue, project: old_project, author: user, moved_to: new_issue) + end + + scenario 'user wants to move issue that has already been moved' do + expect(page).to have_no_select('move_to_project_id') + end + end end def edit_issue(issue) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 7f44ca2f7db..0d6e9cb3a4c 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -130,6 +130,56 @@ describe Issue, models: true do end end + describe '#can_move?' do + let(:user) { create(:user) } + let(:issue) { create(:issue) } + subject { issue.can_move?(user) } + + context 'user is not a member of project issue belongs to' do + it { is_expected.to eq false} + end + + context 'user is reporter in project issue belongs to' do + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project) } + + before { project.team << [user, :reporter] } + + it { is_expected.to eq true } + + context 'checking destination project also' do + subject { issue.can_move?(user, to_project) } + let(:to_project) { create(:project) } + + context 'destination project allowed' do + before { to_project.team << [user, :reporter] } + it { is_expected.to eq true } + end + + context 'destination project not allowed' do + before { to_project.team << [user, :guest] } + it { is_expected.to eq false } + end + end + end + end + + describe '#moved?' do + let(:issue) { create(:issue) } + subject { issue.moved? } + + context 'issue not moved' do + it { is_expected.to eq false } + end + + context 'issue already moved' do + let(:moved_to_issue) { create(:issue) } + let(:issue) { create(:issue, moved_to: moved_to_issue) } + + it { is_expected.to eq true } + end + end + it_behaves_like 'an editable mentionable' do subject { create(:issue) } diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index cb02b8721ac..02ade91ac03 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -91,6 +91,11 @@ describe Issues::MoveService, services: true do it 'creates a new internal id for issue' do expect(new_issue.iid).to be 1 end + + it 'marks issue as moved' do + expect(old_issue.moved?).to eq true + expect(old_issue.moved_to).to eq new_issue + end end context 'issue with notes' do @@ -220,6 +225,19 @@ describe Issues::MoveService, services: true do it { is_expected.to be_falsey } end + + context 'issue has already been moved' do + include_context 'user can move issue' + + let(:moved_to_issue) { create(:issue) } + + let(:old_issue) do + create(:issue, project: old_project, author: author, + moved_to: moved_to_issue) + end + + it { is_expected.to be_falsey } + end end end end