summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-09-13 13:45:09 +0000
committerRémy Coutable <remy@rymai.me>2016-09-13 13:45:09 +0000
commit404f438f45c2da884cf4f16239ac1fda548db09f (patch)
treeef6c8c2570a8fd8d268c9f3ac7c65fe86d84d953
parenta0c46221626ae367c3da68e38a6d5dde7dda32db (diff)
parent66e92895e3c62a53c10ac93148d5f4fd963c93b2 (diff)
downloadgitlab-ce-404f438f45c2da884cf4f16239ac1fda548db09f.tar.gz
Merge branch '21636-deleting-source-project-with-existing-fork-closes-all-related-merge-requests' into 'master'
Deleting source project with existing fork link should closes all related merge requests, and does not render `invalid` template for merge requests with deleted source project. The merge request without source project can be edited but can't be reopened. Closes #21636 See merge request !6177
-rw-r--r--CHANGELOG1
-rw-r--r--app/controllers/projects/merge_requests_controller.rb6
-rw-r--r--app/models/merge_request.rb10
-rw-r--r--app/services/projects/destroy_service.rb2
-rw-r--r--app/views/projects/merge_requests/_discussion.html.haml2
-rw-r--r--app/views/projects/merge_requests/_show.html.haml31
-rw-r--r--spec/controllers/projects_controller_spec.rb19
-rw-r--r--spec/models/merge_request_spec.rb77
8 files changed, 127 insertions, 21 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 195362046dc..4cad8707015 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -127,6 +127,7 @@ v 8.12.0 (unreleased)
- Allow bulk update merge requests from merge requests index page
- Add notification_settings API calls !5632 (mahcsig)
- Remove duplication between project builds and admin builds view !5680 (Katarzyna Kobierska Ula Budziszewska)
+ - Deleting source project with existing fork link will close all related merge requests !6177 (Katarzyna Kobierska Ula Budziszeska)
v 8.11.6 (unreleased)
- Fix an error where we were unable to create a CommitStatus for running state
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 8895cb955bd..aa8645ba8cc 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -428,17 +428,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def validates_merge_request
- # If source project was removed (Ex. mr from fork to origin)
- return invalid_mr unless @merge_request.source_project
-
# Show git not found page
# if there is no saved commits between source & target branch
if @merge_request.commits.blank?
# and if target branch doesn't exist
return invalid_mr unless @merge_request.target_branch_exists?
-
- # or if source branch doesn't exist
- return invalid_mr unless @merge_request.source_branch_exists?
end
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b0b1313f94a..f7d1253d957 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -316,6 +316,10 @@ class MergeRequest < ActiveRecord::Base
closed? && forked_source_project_missing?
end
+ def closed_without_source_project?
+ closed? && !source_project
+ end
+
def forked_source_project_missing?
return false unless for_fork?
return true unless source_project
@@ -323,6 +327,12 @@ class MergeRequest < ActiveRecord::Base
!source_project.forked_from?(target_project)
end
+ def reopenable?
+ return false if closed_without_fork? || closed_without_source_project? || merged?
+
+ closed?
+ end
+
def ensure_merge_request_diff
merge_request_diff || create_merge_request_diff
end
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 8a53f65aec1..a08c6fcd94b 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -27,6 +27,8 @@ module Projects
# Git data (e.g. a list of branch names).
flush_caches(project, wiki_path)
+ Projects::UnlinkForkService.new(project, current_user).execute
+
Project.transaction do
project.destroy!
diff --git a/app/views/projects/merge_requests/_discussion.html.haml b/app/views/projects/merge_requests/_discussion.html.haml
index d070979bcfe..3900b4f6f17 100644
--- a/app/views/projects/merge_requests/_discussion.html.haml
+++ b/app/views/projects/merge_requests/_discussion.html.haml
@@ -2,7 +2,7 @@
- if can?(current_user, :update_merge_request, @merge_request)
- if @merge_request.open?
= link_to 'Close merge request', merge_request_path(@merge_request, merge_request: {state_event: :close }), method: :put, class: "btn btn-nr btn-comment btn-close close-mr-link js-note-target-close", title: "Close merge request", data: {original_text: "Close merge request", alternative_text: "Comment & close merge request"}
- - if @merge_request.closed?
+ - if @merge_request.reopenable?
= link_to 'Reopen merge request', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "btn btn-nr btn-comment btn-reopen reopen-mr-link js-note-target-reopen", title: "Reopen merge request", data: {original_text: "Reopen merge request", alternative_text: "Comment & reopen merge request"}
%comment-and-resolve-btn{ "inline-template" => true, ":discussion-id" => "" }
%button.btn.btn-nr.btn-default.append-right-10.js-comment-resolve-button{ "v-if" => "showButton", type: "submit", data: { namespace_path: "#{@merge_request.project.namespace.path}", project_path: "#{@merge_request.project.path}" } }
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 4b4d418e8ec..d03ff9ec7e8 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -29,17 +29,19 @@
%ul.dropdown-menu.dropdown-menu-align-right
%li= link_to "Email Patches", merge_request_path(@merge_request, format: :patch)
%li= link_to "Plain Diff", merge_request_path(@merge_request, format: :diff)
- .normal
- %span Request to merge
- %span.label-branch= source_branch_with_namespace(@merge_request)
- %span into
- %span.label-branch
- = link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch)
- - if @merge_request.open? && @merge_request.diverged_from_target_branch?
- %span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind)
+ - unless @merge_request.closed_without_fork?
+ .normal
+ %span Request to merge
+ %span.label-branch= source_branch_with_namespace(@merge_request)
+ %span into
+ %span.label-branch
+ = link_to @merge_request.target_branch, namespace_project_commits_path(@project.namespace, @project, @merge_request.target_branch)
+ - if @merge_request.open? && @merge_request.diverged_from_target_branch?
+ %span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind)
- = render "projects/merge_requests/show/how_to_merge"
- = render "projects/merge_requests/widget/show.html.haml"
+ - unless @merge_request.closed_without_source_project?
+ = render "projects/merge_requests/show/how_to_merge"
+ = render "projects/merge_requests/widget/show.html.haml"
- if @merge_request.source_branch_exists? && @merge_request.mergeable? && @merge_request.can_be_merged_by?(current_user)
.light.prepend-top-default.append-bottom-default
@@ -53,10 +55,11 @@
= link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do
Discussion
%span.badge= @merge_request.mr_and_commit_notes.user.count
- %li.commits-tab
- = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do
- Commits
- %span.badge= @commits_count
+ - unless @merge_request.closed_without_source_project?
+ %li.commits-tab
+ = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do
+ Commits
+ %span.badge= @commits_count
- if @pipeline
%li.pipelines-tab
= link_to pipelines_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: '#pipelines', action: 'pipelines', toggle: 'tab' } do
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index ffe0641ddd7..b0f740f48f7 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -181,6 +181,25 @@ describe ProjectsController do
expect(response).to have_http_status(302)
expect(response).to redirect_to(dashboard_projects_path)
end
+
+ context "when the project is forked" do
+ let(:project) { create(:project) }
+ let(:fork_project) { create(:project, forked_from_project: project) }
+ let(:merge_request) do
+ create(:merge_request,
+ source_project: fork_project,
+ target_project: project)
+ end
+
+ it "closes all related merge requests" do
+ project.merge_requests << merge_request
+ sign_in(admin)
+
+ delete :destroy, namespace_id: fork_project.namespace.path, id: fork_project.path
+
+ expect(merge_request.reload.state).to eq('closed')
+ end
+ end
end
describe "POST #toggle_star" do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 5bf3b8e609e..3b815ded2d3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1038,4 +1038,81 @@ describe MergeRequest, models: true do
end
end
end
+
+ describe '#closed_without_source_project?' do
+ let(:project) { create(:project) }
+ let(:user) { create(:user) }
+ let(:fork_project) { create(:project, forked_from_project: project, namespace: user.namespace) }
+ let(:destroy_service) { Projects::DestroyService.new(fork_project, user) }
+
+ context 'when the merge request is closed' do
+ let(:closed_merge_request) do
+ create(:closed_merge_request,
+ source_project: fork_project,
+ target_project: project)
+ end
+
+ it 'returns false if the source project exists' do
+ expect(closed_merge_request.closed_without_source_project?).to be_falsey
+ end
+
+ it 'returns true if the source project does not exist' do
+ destroy_service.execute
+ closed_merge_request.reload
+
+ expect(closed_merge_request.closed_without_source_project?).to be_truthy
+ end
+ end
+
+ context 'when the merge request is open' do
+ it 'returns false' do
+ expect(subject.closed_without_source_project?).to be_falsey
+ end
+ end
+ end
+
+ describe '#reopenable?' do
+ context 'when the merge request is closed' do
+ it 'returns true' do
+ subject.close
+
+ expect(subject.reopenable?).to be_truthy
+ end
+
+ context 'forked project' do
+ let(:project) { create(:project) }
+ let(:user) { create(:user) }
+ let(:fork_project) { create(:project, forked_from_project: project, namespace: user.namespace) }
+ let(:merge_request) do
+ create(:closed_merge_request,
+ source_project: fork_project,
+ target_project: project)
+ end
+
+ it 'returns false if unforked' do
+ Projects::UnlinkForkService.new(fork_project, user).execute
+
+ expect(merge_request.reload.reopenable?).to be_falsey
+ end
+
+ it 'returns false if the source project is deleted' do
+ Projects::DestroyService.new(fork_project, user).execute
+
+ expect(merge_request.reload.reopenable?).to be_falsey
+ end
+
+ it 'returns false if the merge request is merged' do
+ merge_request.update_attributes(state: 'merged')
+
+ expect(merge_request.reload.reopenable?).to be_falsey
+ end
+ end
+ end
+
+ context 'when the merge request is opened' do
+ it 'returns false' do
+ expect(subject.reopenable?).to be_falsey
+ end
+ end
+ end
end