Commit 41396031 authored by Robert Speicher's avatar Robert Speicher

Merge branch '29541-fix-github-importer-deleted-fork' into 'master'

Fix GitHub importer for PRs of deleted forked repositories

Closes #29541

See merge request !9992
parents ff3703f5 d5f34094
---
title: Fix GitHub Importer for PRs of deleted forked repositories
merge_request: 9992
author:
......@@ -11,6 +11,14 @@ module Gitlab
sha.present? && ref.present?
end
def user
raw_data.user&.login || 'unknown'
end
def short_sha
Commit.truncate_sha(sha)
end
private
def branch_exists?
......
module Gitlab
module GithubImport
class PullRequestFormatter < IssuableFormatter
delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true
delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true
delegate :user, :project, :ref, :repo, :sha, to: :source_branch, prefix: true
delegate :user, :exists?, :project, :ref, :repo, :sha, :short_sha, to: :target_branch, prefix: true
def attributes
{
......@@ -37,13 +37,20 @@ module Gitlab
end
def source_branch_name
@source_branch_name ||= begin
if cross_project?
"pull/#{number}/#{source_branch_repo.full_name}/#{source_branch_ref}"
@source_branch_name ||=
if cross_project? || !source_branch_exists?
source_branch_name_prefixed
else
source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}"
source_branch_ref
end
end
def source_branch_name_prefixed
"gh-#{target_branch_short_sha}/#{number}/#{source_branch_user}/#{source_branch_ref}"
end
def source_branch_exists?
!cross_project? && source_branch.exists?
end
def target_branch
......@@ -51,13 +58,17 @@ module Gitlab
end
def target_branch_name
@target_branch_name ||= begin
target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}"
@target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed
end
def target_branch_name_prefixed
"gl-#{target_branch_short_sha}/#{number}/#{target_branch_user}/#{target_branch_ref}"
end
def cross_project?
source_branch.repo.id != target_branch.repo.id
return true if source_branch_repo.nil?
source_branch_repo.id != target_branch_repo.id
end
def opened?
......
......@@ -215,9 +215,9 @@ describe Gitlab::GithubImport::Importer, lib: true do
let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
let(:repository) { double(id: 1, fork: false) }
let(:source_sha) { create(:commit, project: project).id }
let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha) }
let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha, user: octocat) }
let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) }
let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha, user: octocat) }
let(:pull_request) do
double(
number: 1347,
......
......@@ -4,15 +4,18 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
let(:client) { double }
let(:project) { create(:project, :repository) }
let(:source_sha) { create(:commit, project: project).id }
let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
let(:target_commit) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit) }
let(:target_sha) { target_commit.id }
let(:target_short_sha) { target_commit.id.to_s[0..7] }
let(:repository) { double(id: 1, fork: false) }
let(:source_repo) { repository }
let(:source_branch) { double(ref: 'branch-merged', repo: source_repo, sha: source_sha) }
let(:forked_source_repo) { double(id: 2, fork: true, name: 'otherproject', full_name: 'company/otherproject') }
let(:target_repo) { repository }
let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) }
let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha, user: octocat) }
let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) }
let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) }
let(:branch_deleted_repo) { double(ref: 'master', repo: nil, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) }
let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') }
let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
......@@ -203,16 +206,24 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
context 'when source branch does not exist' do
let(:raw_data) { double(base_data.merge(head: removed_branch)) }
it 'prefixes branch name with pull request number' do
expect(pull_request.source_branch_name).to eq 'pull/1347/removed-branch'
it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do
expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/removed-branch"
end
end
context 'when source branch is from a fork' do
let(:raw_data) { double(base_data.merge(head: forked_branch)) }
it 'prefixes branch name with pull request number and project with namespace to avoid collision' do
expect(pull_request.source_branch_name).to eq 'pull/1347/company/otherproject/master'
it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do
expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master"
end
end
context 'when source branch is from a deleted fork' do
let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) }
it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do
expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master"
end
end
end
......@@ -229,8 +240,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
context 'when target branch does not exist' do
let(:raw_data) { double(base_data.merge(base: removed_branch)) }
it 'prefixes branch name with pull request number' do
expect(pull_request.target_branch_name).to eq 'pull/1347/removed-branch'
it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do
expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347/octocat/removed-branch'
end
end
end
......@@ -290,6 +301,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
end
end
context 'when source repository does not exist anymore' do
let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) }
it 'returns true' do
expect(pull_request.cross_project?).to eq true
end
end
context 'when source and target repositories are the same' do
let(:raw_data) { double(base_data.merge(head: source_branch)) }
......@@ -299,6 +318,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
end
end
describe '#source_branch_exists?' do
let(:raw_data) { double(base_data.merge(head: forked_branch)) }
it 'returns false when is a cross_project' do
expect(pull_request.source_branch_exists?).to eq false
end
end
describe '#url' do
let(:raw_data) { double(base_data) }
......
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