Commit 030c8226 authored by Douwe Maan's avatar Douwe Maan

Merge branch '23341-fix-viewing-mr-from-deleted-project' into 'master'

Fix a 500 error viewing an MR with a deleted source project

## What does this MR do?

Allows merged MRs to be shown without any 500 errors if the source project is removed

## Are there points in the code the reviewer needs to double check?

https://gitlab.com/gitlab-org/gitlab-ce/commit/31c37c6c38258684fc92e0d91119c33872e39034 fixed this for closed MRs only. I had trouble understanding the introduced helper and logic, so reverted it and keyed everything on the existence of the source project or branch directly.

commits.json returns a 500 error for a closed or merged MR; the approach taken in the above MR was to hide the commits... tab, so I've run with that.

For merged MRs, the commits (but not the pipeline data) are in the target project, so we *could* do better, but it's a fairly nasty intervention to make it happen.

## Why was this MR needed?

Viewing merged MRs should work even if the fork they came from has been deleted or unlinked.

## Screenshots (if relevant)

![Screen_Shot_2016-10-19_at_17.56.37](/uploads/1aeadd5147b9a4ad29b946b1c7ea52cb/Screen_Shot_2016-10-19_at_17.56.37.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #23341

See merge request !6991
parents 607fb9b2 0bbfdde6
...@@ -39,6 +39,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -39,6 +39,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- ProjectCacheWorker updates caches at most once per 15 minutes per project - ProjectCacheWorker updates caches at most once per 15 minutes per project
- Fix Error 500 when viewing old merge requests with bad diff data - Fix Error 500 when viewing old merge requests with bad diff data
- Create a new /templates namespace for the /licenses, /gitignores and /gitlab_ci_ymls API endpoints. !5717 (tbalthazar) - Create a new /templates namespace for the /licenses, /gitignores and /gitlab_ci_ymls API endpoints. !5717 (tbalthazar)
- Fix viewing merged MRs when the source project has been removed !6991
- Speed-up group milestones show page - Speed-up group milestones show page
- Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps) - Fix inconsistent options dropdown caret on mobile viewports (ClemMakesApps)
- Extract project#update_merge_requests and SystemHooks to its own worker from GitPushService - Extract project#update_merge_requests and SystemHooks to its own worker from GitPushService
......
...@@ -398,7 +398,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -398,7 +398,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
status ||= "preparing" status ||= "preparing"
else else
ci_service = @merge_request.source_project.ci_service ci_service = @merge_request.source_project.try(:ci_service)
status = ci_service.commit_status(merge_request.diff_head_sha, merge_request.source_branch) if ci_service status = ci_service.commit_status(merge_request.diff_head_sha, merge_request.source_branch) if ci_service
if ci_service.respond_to?(:commit_coverage) if ci_service.respond_to?(:commit_coverage)
...@@ -554,7 +554,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -554,7 +554,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def define_pipelines_vars def define_pipelines_vars
@pipelines = @merge_request.all_pipelines @pipelines = @merge_request.all_pipelines
if @pipelines.any? if @pipelines.present?
@pipeline = @pipelines.first @pipeline = @pipelines.first
@statuses = @pipeline.statuses.relevant @statuses = @pipeline.statuses.relevant
end end
......
...@@ -86,11 +86,15 @@ module MergeRequestsHelper ...@@ -86,11 +86,15 @@ module MergeRequestsHelper
end end
def source_branch_with_namespace(merge_request) def source_branch_with_namespace(merge_request)
branch = link_to(merge_request.source_branch, namespace_project_commits_path(merge_request.source_project.namespace, merge_request.source_project, merge_request.source_branch)) namespace = merge_request.source_project_namespace
branch = merge_request.source_branch
if merge_request.source_branch_exists?
namespace = link_to(namespace, project_path(merge_request.source_project))
branch = link_to(branch, namespace_project_commits_path(merge_request.source_project.namespace, merge_request.source_project, merge_request.source_branch))
end
if merge_request.for_fork? if merge_request.for_fork?
namespace = link_to(merge_request.source_project_namespace,
project_path(merge_request.source_project))
namespace + ":" + branch namespace + ":" + branch
else else
branch branch
......
...@@ -326,21 +326,17 @@ class MergeRequest < ActiveRecord::Base ...@@ -326,21 +326,17 @@ class MergeRequest < ActiveRecord::Base
def validate_fork def validate_fork
return true unless target_project && source_project return true unless target_project && source_project
return true if target_project == source_project return true if target_project == source_project
return true unless forked_source_project_missing? return true unless source_project_missing?
errors.add :validate_fork, errors.add :validate_fork,
'Source project is not a fork of the target project' 'Source project is not a fork of the target project'
end end
def closed_without_fork? def closed_without_fork?
closed? && forked_source_project_missing? closed? && source_project_missing?
end end
def closed_without_source_project? def source_project_missing?
closed? && !source_project
end
def forked_source_project_missing?
return false unless for_fork? return false unless for_fork?
return true unless source_project return true unless source_project
...@@ -348,9 +344,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -348,9 +344,7 @@ class MergeRequest < ActiveRecord::Base
end end
def reopenable? def reopenable?
return false if closed_without_fork? || closed_without_source_project? || merged? closed? && !source_project_missing? && source_branch_exists?
closed?
end end
def ensure_merge_request_diff def ensure_merge_request_diff
...@@ -662,7 +656,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -662,7 +656,7 @@ class MergeRequest < ActiveRecord::Base
end end
def has_ci? def has_ci?
source_project.ci_service && commits.any? source_project.try(:ci_service) && commits.any?
end end
def branch_missing? def branch_missing?
...@@ -694,12 +688,9 @@ class MergeRequest < ActiveRecord::Base ...@@ -694,12 +688,9 @@ class MergeRequest < ActiveRecord::Base
@environments ||= @environments ||=
begin begin
environments = source_project.environments_for( envs = target_project.environments_for(target_branch, diff_head_commit, with_tags: true)
source_branch, diff_head_commit) envs.concat(source_project.environments_for(source_branch, diff_head_commit)) if source_project
environments += target_project.environments_for( envs.uniq
target_branch, diff_head_commit, with_tags: true)
environments.uniq
end end
end end
......
...@@ -26,7 +26,6 @@ ...@@ -26,7 +26,6 @@
%ul.dropdown-menu.dropdown-menu-align-right %ul.dropdown-menu.dropdown-menu-align-right
%li= link_to "Email Patches", merge_request_path(@merge_request, format: :patch) %li= link_to "Email Patches", merge_request_path(@merge_request, format: :patch)
%li= link_to "Plain Diff", merge_request_path(@merge_request, format: :diff) %li= link_to "Plain Diff", merge_request_path(@merge_request, format: :diff)
- unless @merge_request.closed_without_fork?
.normal .normal
%span Request to merge %span Request to merge
%span.label-branch= source_branch_with_namespace(@merge_request) %span.label-branch= source_branch_with_namespace(@merge_request)
...@@ -36,8 +35,9 @@ ...@@ -36,8 +35,9 @@
- if @merge_request.open? && @merge_request.diverged_from_target_branch? - if @merge_request.open? && @merge_request.diverged_from_target_branch?
%span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind) %span (#{pluralize(@merge_request.diverged_commits_count, 'commit')} behind)
- unless @merge_request.closed_without_source_project? - if @merge_request.source_branch_exists?
= render "projects/merge_requests/show/how_to_merge" = render "projects/merge_requests/show/how_to_merge"
= render "projects/merge_requests/widget/show.html.haml" = 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) - if @merge_request.source_branch_exists? && @merge_request.mergeable? && @merge_request.can_be_merged_by?(current_user)
...@@ -52,7 +52,7 @@ ...@@ -52,7 +52,7 @@
= link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do
Discussion Discussion
%span.badge= @merge_request.mr_and_commit_notes.user.count %span.badge= @merge_request.mr_and_commit_notes.user.count
- unless @merge_request.closed_without_source_project? - if @merge_request.source_project
%li.commits-tab %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 = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do
Commits Commits
......
...@@ -25,6 +25,20 @@ feature 'Merge request created from fork' do ...@@ -25,6 +25,20 @@ feature 'Merge request created from fork' do
expect(page).to have_content 'Test merge request' expect(page).to have_content 'Test merge request'
end end
context 'source project is deleted' do
background do
MergeRequests::MergeService.new(project, user).execute(merge_request)
fork_project.destroy!
end
scenario 'user can access merge request' do
visit_merge_request(merge_request)
expect(page).to have_content 'Test merge request'
expect(page).to have_content "(removed):#{merge_request.source_branch}"
end
end
context 'pipeline present in source project' do context 'pipeline present in source project' do
include WaitForAjax include WaitForAjax
......
...@@ -1198,7 +1198,7 @@ describe MergeRequest, models: true do ...@@ -1198,7 +1198,7 @@ describe MergeRequest, models: true do
end end
end end
describe "#forked_source_project_missing?" do describe "#source_project_missing?" do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:fork_project) { create(:project, forked_from_project: project) } let(:fork_project) { create(:project, forked_from_project: project) }
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -1211,13 +1211,13 @@ describe MergeRequest, models: true do ...@@ -1211,13 +1211,13 @@ describe MergeRequest, models: true do
target_project: project) target_project: project)
end end
it { expect(merge_request.forked_source_project_missing?).to be_falsey } it { expect(merge_request.source_project_missing?).to be_falsey }
end end
context "when the source project is the same as the target project" do context "when the source project is the same as the target project" do
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
it { expect(merge_request.forked_source_project_missing?).to be_falsey } it { expect(merge_request.source_project_missing?).to be_falsey }
end end
context "when the fork does not exist" do context "when the fork does not exist" do
...@@ -1231,7 +1231,7 @@ describe MergeRequest, models: true do ...@@ -1231,7 +1231,7 @@ describe MergeRequest, models: true do
unlink_project.execute unlink_project.execute
merge_request.reload merge_request.reload
expect(merge_request.forked_source_project_missing?).to be_truthy expect(merge_request.source_project_missing?).to be_truthy
end end
end end
end end
...@@ -1274,38 +1274,6 @@ describe MergeRequest, models: true do ...@@ -1274,38 +1274,6 @@ describe MergeRequest, models: true do
end 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 describe '#reopenable?' do
context 'when the merge request is closed' do context 'when the merge request is closed' do
it 'returns true' do it 'returns true' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment