From e8c723543cfc4c1d905a5794a2da1bef7689d784 Mon Sep 17 00:00:00 2001 From: Baldinof Date: Wed, 9 Mar 2016 15:25:48 +0100 Subject: [PATCH 1/4] Close merge requests when removing fork relation --- CHANGELOG | 1 + app/controllers/projects_controller.rb | 2 +- app/models/merge_request.rb | 1 + app/models/project.rb | 12 +++++++++++- spec/models/project_spec.rb | 19 +++++++++++++++++++ 5 files changed, 33 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index c1c90903d5d..84739fab82b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.6.0 (unreleased) - Show labels in dashboard and group milestone views - Add main language of a project in the list of projects (Tiago Botelho) - Add ability to show archived projects on dashboard, explore and group pages + - Remove fork link closes all merge requests opened on source project (Florent Baldino) v 8.5.5 - Ensure removing a project removes associated Todo entries diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index aea08ecce3e..a26d11459f0 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -72,7 +72,7 @@ class ProjectsController < ApplicationController def remove_fork return access_denied! unless can?(current_user, :remove_fork_project, @project) - if @project.unlink_fork + if @project.unlink_fork(current_user) flash[:notice] = 'The fork relationship has been removed.' end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c1e18bb3cc5..18ec48b57f4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -137,6 +137,7 @@ class MergeRequest < ActiveRecord::Base scope :by_milestone, ->(milestone) { where(milestone_id: milestone) } scope :in_projects, ->(project_ids) { where("source_project_id in (:project_ids) OR target_project_id in (:project_ids)", project_ids: project_ids) } scope :of_projects, ->(ids) { where(target_project_id: ids) } + scope :from_project, ->(project) { where(source_project_id: project.id) } scope :merged, -> { with_state(:merged) } scope :closed_and_merged, -> { with_states(:closed, :merged) } diff --git a/app/models/project.rb b/app/models/project.rb index 65829bec77a..859758293e1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -890,12 +890,22 @@ class Project < ActiveRecord::Base self.builds_enabled = true end - def unlink_fork + def unlink_fork(user) if forked? forked_from_project.lfs_objects.find_each do |lfs_object| lfs_object.projects << self end + merge_requests = forked_from_project.merge_requests.opened.from_project(self) + + unless merge_requests.empty? + close_service = MergeRequests::CloseService.new(self, user) + + merge_requests.each do |mr| + close_service.execute(mr) + end + end + forked_project_link.destroy end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2fa38a5d3d3..ba4fb2f8222 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -647,4 +647,23 @@ describe Project, models: true do project.expire_caches_before_rename('foo') end end + + describe '#unlink_fork' do + let(:fork_link) { create(:forked_project_link) } + let(:fork_project) { fork_link.forked_to_project } + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: fork_link.forked_from_project) } + let!(:close_service) { MergeRequests::CloseService.new(fork_project, user) } + + it 'remove fork relation and close all pending merge requests' do + allow(MergeRequests::CloseService).to receive(:new). + with(fork_project, user). + and_return(close_service) + + expect(close_service).to receive(:execute).with(merge_request) + expect(fork_project.forked_project_link).to receive(:destroy) + + fork_project.unlink_fork(user) + end + end end From fa4126acffdfe13741e05a60ad5ed7fd407b4f16 Mon Sep 17 00:00:00 2001 From: Baldinof Date: Tue, 22 Mar 2016 15:34:35 +0100 Subject: [PATCH 2/4] Move unlink fork logic to a service --- app/controllers/projects_controller.rb | 2 +- app/models/project.rb | 20 ------------ app/services/projects/unlink_fork_service.rb | 19 +++++++++++ spec/models/project_spec.rb | 19 ----------- .../projects/unlink_fork_service_spec.rb | 32 +++++++++++++++++++ 5 files changed, 52 insertions(+), 40 deletions(-) create mode 100644 app/services/projects/unlink_fork_service.rb create mode 100644 spec/services/projects/unlink_fork_service_spec.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 94789702d65..87657e4e3d2 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -71,7 +71,7 @@ class ProjectsController < ApplicationController def remove_fork return access_denied! unless can?(current_user, :remove_fork_project, @project) - if @project.unlink_fork(current_user) + if ::Projects::UnlinkForkService.new(@project, current_user).execute flash[:notice] = 'The fork relationship has been removed.' end end diff --git a/app/models/project.rb b/app/models/project.rb index 8d9908128e2..691b706ea40 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -917,26 +917,6 @@ class Project < ActiveRecord::Base self.builds_enabled = true end - def unlink_fork(user) - if forked? - forked_from_project.lfs_objects.find_each do |lfs_object| - lfs_object.projects << self - end - - merge_requests = forked_from_project.merge_requests.opened.from_project(self) - - unless merge_requests.empty? - close_service = MergeRequests::CloseService.new(self, user) - - merge_requests.each do |mr| - close_service.execute(mr) - end - end - - forked_project_link.destroy - end - end - def any_runners?(&block) if runners.active.any?(&block) return true diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb new file mode 100644 index 00000000000..d0703effa1d --- /dev/null +++ b/app/services/projects/unlink_fork_service.rb @@ -0,0 +1,19 @@ +module Projects + class UnlinkForkService < BaseService + def execute + return unless @project.forked? + + @project.forked_from_project.lfs_objects.find_each do |lfs_object| + lfs_object.projects << self + end + + merge_requests = @project.forked_from_project.merge_requests.opened.from_project(@project) + + merge_requests.each do |mr| + MergeRequests::CloseService.new(@project, @current_user).execute(mr) + end + + @project.forked_project_link.destroy + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1ca78daa5b3..59c5ffa6b9c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -699,25 +699,6 @@ describe Project, models: true do end end - describe '#unlink_fork' do - let(:fork_link) { create(:forked_project_link) } - let(:fork_project) { fork_link.forked_to_project } - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: fork_link.forked_from_project) } - let!(:close_service) { MergeRequests::CloseService.new(fork_project, user) } - - it 'remove fork relation and close all pending merge requests' do - allow(MergeRequests::CloseService).to receive(:new). - with(fork_project, user). - and_return(close_service) - - expect(close_service).to receive(:execute).with(merge_request) - expect(fork_project.forked_project_link).to receive(:destroy) - - fork_project.unlink_fork(user) - end - end - describe '.search_by_title' do let(:project) { create(:project, name: 'kittens') } diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb new file mode 100644 index 00000000000..f287b0a59b2 --- /dev/null +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Projects::UnlinkForkService, services: true do + subject { Projects::UnlinkForkService.new(fork_project, user) } + + let(:fork_link) { create(:forked_project_link) } + let(:fork_project) { fork_link.forked_to_project } + let(:user) { create(:user) } + + context 'with opened merge request on the source project' do + let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: fork_link.forked_from_project) } + let(:mr_close_service) { MergeRequests::CloseService.new(fork_project, user) } + + before do + allow(MergeRequests::CloseService).to receive(:new). + with(fork_project, user). + and_return(mr_close_service) + end + + it 'close all pending merge requests' do + expect(mr_close_service).to receive(:execute).with(merge_request) + + subject.execute + end + end + + it 'remove fork relation' do + expect(fork_project.forked_project_link).to receive(:destroy) + + subject.execute + end +end From 65fa7161736f6688719b6e66952c611770a64473 Mon Sep 17 00:00:00 2001 From: Baldinof Date: Sun, 3 Apr 2016 15:17:14 +0000 Subject: [PATCH 3/4] Fix rubocop in unlink fork service specs --- spec/services/projects/unlink_fork_service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index f287b0a59b2..23f5555d3e0 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -25,8 +25,8 @@ describe Projects::UnlinkForkService, services: true do end it 'remove fork relation' do - expect(fork_project.forked_project_link).to receive(:destroy) + expect(fork_project.forked_project_link).to receive(:destroy) - subject.execute + subject.execute end end From a6b5b50e14885a82530794c6ea35c940305244dd Mon Sep 17 00:00:00 2001 From: Baldinof Date: Mon, 4 Apr 2016 14:41:01 +0000 Subject: [PATCH 4/4] Fix incorrect variable name --- app/services/projects/unlink_fork_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index d0703effa1d..315c3e16292 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -4,7 +4,7 @@ module Projects return unless @project.forked? @project.forked_from_project.lfs_objects.find_each do |lfs_object| - lfs_object.projects << self + lfs_object.projects << @project end merge_requests = @project.forked_from_project.merge_requests.opened.from_project(@project)