summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/services/projects/fork_service.rb36
-rw-r--r--changelogs/unreleased/fj-40752-forks-api-not-using-services.yml5
-rw-r--r--db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb27
-rw-r--r--db/schema.rb2
-rw-r--r--lib/api/projects.rb21
-rw-r--r--spec/services/projects/fork_service_spec.rb353
6 files changed, 271 insertions, 173 deletions
diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb
index eb5cce5ab98..03be7039b2a 100644
--- a/app/services/projects/fork_service.rb
+++ b/app/services/projects/fork_service.rb
@@ -1,6 +1,24 @@
module Projects
class ForkService < BaseService
- def execute
+ def execute(fork_to_project = nil)
+ if fork_to_project
+ link_existing_project(fork_to_project)
+ else
+ fork_new_project
+ end
+ end
+
+ private
+
+ def link_existing_project(fork_to_project)
+ return if fork_to_project.forked?
+
+ link_fork_network(fork_to_project)
+
+ fork_to_project
+ end
+
+ def fork_new_project
new_params = {
forked_from_project_id: @project.id,
visibility_level: allowed_visibility_level,
@@ -21,15 +39,11 @@ module Projects
builds_access_level = @project.project_feature.builds_access_level
new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
- refresh_forks_count
-
link_fork_network(new_project)
new_project
end
- private
-
def fork_network
if @project.fork_network
@project.fork_network
@@ -43,9 +57,17 @@ module Projects
end
end
- def link_fork_network(new_project)
- fork_network.fork_network_members.create(project: new_project,
+ def link_fork_network(fork_to_project)
+ fork_network.fork_network_members.create(project: fork_to_project,
forked_from_project: @project)
+
+ # TODO: remove this when ForkedProjectLink model is removed
+ unless fork_to_project.forked_project_link
+ fork_to_project.create_forked_project_link(forked_to_project: fork_to_project,
+ forked_from_project: @project)
+ end
+
+ refresh_forks_count
end
def refresh_forks_count
diff --git a/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml b/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml
new file mode 100644
index 00000000000..cd7b87596e6
--- /dev/null
+++ b/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml
@@ -0,0 +1,5 @@
+---
+title: Using appropiate services in the API for managing forks
+merge_request: 15709
+author:
+type: fixed
diff --git a/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb b/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb
new file mode 100644
index 00000000000..30ff5173192
--- /dev/null
+++ b/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb
@@ -0,0 +1,27 @@
+class RescheduleForkNetworkCreationCaller < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ MIGRATION = 'PopulateForkNetworksRange'.freeze
+ BATCH_SIZE = 100
+ DELAY_INTERVAL = 15.seconds
+
+ disable_ddl_transaction!
+
+ class ForkedProjectLink < ActiveRecord::Base
+ include EachBatch
+
+ self.table_name = 'forked_project_links'
+ end
+
+ def up
+ say 'Populating the `fork_networks` based on existing `forked_project_links`'
+
+ queue_background_migration_jobs_by_range_at_intervals(ForkedProjectLink, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+ end
+
+ def down
+ # nothing
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 2be1b745342..6ea3ab54742 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20171124150326) do
+ActiveRecord::Schema.define(version: 20171205190711) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 14a4fc6f025..fa222bf2b1c 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -367,15 +367,16 @@ module API
post ":id/fork/:forked_from_id" do
authenticated_as_admin!
- forked_from_project = find_project!(params[:forked_from_id])
- not_found!("Source Project") unless forked_from_project
+ fork_from_project = find_project!(params[:forked_from_id])
- if user_project.forked_from_project.nil?
- user_project.create_forked_project_link(forked_to_project_id: user_project.id, forked_from_project_id: forked_from_project.id)
+ not_found!("Source Project") unless fork_from_project
- ::Projects::ForksCountService.new(forked_from_project).refresh_cache
+ result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project)
+
+ if result
+ present user_project.reload, with: Entities::Project
else
- render_api_error!("Project already forked", 409)
+ render_api_error!("Project already forked", 409) if user_project.forked?
end
end
@@ -383,11 +384,11 @@ module API
delete ":id/fork" do
authorize! :remove_fork_project, user_project
- if user_project.forked?
- destroy_conditionally!(user_project.forked_project_link)
- else
- not_modified!
+ result = destroy_conditionally!(user_project) do
+ ::Projects::UnlinkForkService.new(user_project, current_user).execute
end
+
+ result ? status(204) : not_modified!
end
desc 'Share the project with a group' do
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index 53862283a27..4057caca2ac 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -3,210 +3,253 @@ require 'spec_helper'
describe Projects::ForkService do
include ProjectForksHelper
let(:gitlab_shell) { Gitlab::Shell.new }
+ context 'when forking a new project' do
+ describe 'fork by user' do
+ before do
+ @from_user = create(:user)
+ @from_namespace = @from_user.namespace
+ avatar = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")
+ @from_project = create(:project,
+ :repository,
+ creator_id: @from_user.id,
+ namespace: @from_namespace,
+ star_count: 107,
+ avatar: avatar,
+ description: 'wow such project')
+ @to_user = create(:user)
+ @to_namespace = @to_user.namespace
+ @from_project.add_user(@to_user, :developer)
+ end
- describe 'fork by user' do
- before do
- @from_user = create(:user)
- @from_namespace = @from_user.namespace
- avatar = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")
- @from_project = create(:project,
- :repository,
- creator_id: @from_user.id,
- namespace: @from_namespace,
- star_count: 107,
- avatar: avatar,
- description: 'wow such project')
- @to_user = create(:user)
- @to_namespace = @to_user.namespace
- @from_project.add_user(@to_user, :developer)
- end
+ context 'fork project' do
+ context 'when forker is a guest' do
+ before do
+ @guest = create(:user)
+ @from_project.add_user(@guest, :guest)
+ end
+ subject { fork_project(@from_project, @guest) }
- context 'fork project' do
- context 'when forker is a guest' do
- before do
- @guest = create(:user)
- @from_project.add_user(@guest, :guest)
+ it { is_expected.not_to be_persisted }
+ it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) }
end
- subject { fork_project(@from_project, @guest) }
- it { is_expected.not_to be_persisted }
- it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) }
- end
+ describe "successfully creates project in the user namespace" do
+ let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace) }
- describe "successfully creates project in the user namespace" do
- let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace) }
-
- it { expect(to_project).to be_persisted }
- it { expect(to_project.errors).to be_empty }
- it { expect(to_project.owner).to eq(@to_user) }
- it { expect(to_project.namespace).to eq(@to_user.namespace) }
- it { expect(to_project.star_count).to be_zero }
- it { expect(to_project.description).to eq(@from_project.description) }
- it { expect(to_project.avatar.file).to be_exists }
-
- # This test is here because we had a bug where the from-project lost its
- # avatar after being forked.
- # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158
- it "after forking the from-project still has its avatar" do
- # If we do not fork the project first we cannot detect the bug.
- expect(to_project).to be_persisted
-
- expect(@from_project.avatar.file).to be_exists
- end
+ it { expect(to_project).to be_persisted }
+ it { expect(to_project.errors).to be_empty }
+ it { expect(to_project.owner).to eq(@to_user) }
+ it { expect(to_project.namespace).to eq(@to_user.namespace) }
+ it { expect(to_project.star_count).to be_zero }
+ it { expect(to_project.description).to eq(@from_project.description) }
+ it { expect(to_project.avatar.file).to be_exists }
- it 'flushes the forks count cache of the source project' do
- expect(@from_project.forks_count).to be_zero
+ # This test is here because we had a bug where the from-project lost its
+ # avatar after being forked.
+ # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158
+ it "after forking the from-project still has its avatar" do
+ # If we do not fork the project first we cannot detect the bug.
+ expect(to_project).to be_persisted
- fork_project(@from_project, @to_user)
+ expect(@from_project.avatar.file).to be_exists
+ end
- expect(@from_project.forks_count).to eq(1)
- end
+ it 'flushes the forks count cache of the source project' do
+ expect(@from_project.forks_count).to be_zero
- it 'creates a fork network with the new project and the root project set' do
- to_project
- fork_network = @from_project.reload.fork_network
+ fork_project(@from_project, @to_user)
- expect(fork_network).not_to be_nil
- expect(fork_network.root_project).to eq(@from_project)
- expect(fork_network.projects).to contain_exactly(@from_project, to_project)
- end
- end
+ expect(@from_project.forks_count).to eq(1)
+ end
- context 'creating a fork of a fork' do
- let(:from_forked_project) { fork_project(@from_project, @to_user) }
- let(:other_namespace) do
- group = create(:group)
- group.add_owner(@to_user)
- group
- end
- let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) }
+ it 'creates a fork network with the new project and the root project set' do
+ to_project
+ fork_network = @from_project.reload.fork_network
- it 'sets the root of the network to the root project' do
- expect(to_project.fork_network.root_project).to eq(@from_project)
+ expect(fork_network).not_to be_nil
+ expect(fork_network.root_project).to eq(@from_project)
+ expect(fork_network.projects).to contain_exactly(@from_project, to_project)
+ end
end
- it 'sets the forked_from_project on the membership' do
- expect(to_project.fork_network_member.forked_from_project).to eq(from_forked_project)
+ context 'creating a fork of a fork' do
+ let(:from_forked_project) { fork_project(@from_project, @to_user) }
+ let(:other_namespace) do
+ group = create(:group)
+ group.add_owner(@to_user)
+ group
+ end
+ let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) }
+
+ it 'sets the root of the network to the root project' do
+ expect(to_project.fork_network.root_project).to eq(@from_project)
+ end
+
+ it 'sets the forked_from_project on the membership' do
+ expect(to_project.fork_network_member.forked_from_project).to eq(from_forked_project)
+ end
end
end
- end
- context 'project already exists' do
- it "fails due to validation, not transaction failure" do
- @existing_project = create(:project, :repository, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace)
- @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace)
- expect(@existing_project).to be_persisted
+ context 'project already exists' do
+ it "fails due to validation, not transaction failure" do
+ @existing_project = create(:project, :repository, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace)
+ @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace)
+ expect(@existing_project).to be_persisted
- expect(@to_project).not_to be_persisted
- expect(@to_project.errors[:name]).to eq(['has already been taken'])
- expect(@to_project.errors[:path]).to eq(['has already been taken'])
+ expect(@to_project).not_to be_persisted
+ expect(@to_project.errors[:name]).to eq(['has already been taken'])
+ expect(@to_project.errors[:path]).to eq(['has already been taken'])
+ end
end
- end
- context 'repository already exists' do
- let(:repository_storage) { 'default' }
- let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
+ context 'repository already exists' do
+ let(:repository_storage) { 'default' }
+ let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
- before do
- gitlab_shell.add_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}")
- end
+ before do
+ gitlab_shell.add_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}")
+ end
- after do
- gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}")
- end
+ after do
+ gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}")
+ end
- it 'does not allow creation' do
- to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace)
+ it 'does not allow creation' do
+ to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace)
- expect(to_project).not_to be_persisted
- expect(to_project.errors.messages).to have_key(:base)
- expect(to_project.errors.messages[:base].first).to match('There is already a repository with that name on disk')
+ expect(to_project).not_to be_persisted
+ expect(to_project.errors.messages).to have_key(:base)
+ expect(to_project.errors.messages[:base].first).to match('There is already a repository with that name on disk')
+ end
end
- end
- context 'GitLab CI is enabled' do
- it "forks and enables CI for fork" do
- @from_project.enable_ci
- @to_project = fork_project(@from_project, @to_user)
- expect(@to_project.builds_enabled?).to be_truthy
+ context 'GitLab CI is enabled' do
+ it "forks and enables CI for fork" do
+ @from_project.enable_ci
+ @to_project = fork_project(@from_project, @to_user)
+ expect(@to_project.builds_enabled?).to be_truthy
+ end
end
- end
- context "when project has restricted visibility level" do
- context "and only one visibility level is restricted" do
- before do
- @from_project.update_attributes(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
- stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL])
+ context "when project has restricted visibility level" do
+ context "and only one visibility level is restricted" do
+ before do
+ @from_project.update_attributes(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+ stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL])
+ end
+
+ it "creates fork with highest allowed level" do
+ forked_project = fork_project(@from_project, @to_user)
+
+ expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC)
+ end
end
- it "creates fork with highest allowed level" do
- forked_project = fork_project(@from_project, @to_user)
+ context "and all visibility levels are restricted" do
+ before do
+ stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PRIVATE])
+ end
+
+ it "creates fork with private visibility levels" do
+ forked_project = fork_project(@from_project, @to_user)
- expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC)
+ expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+ end
end
end
+ end
- context "and all visibility levels are restricted" do
- before do
- stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PRIVATE])
+ describe 'fork to namespace' do
+ before do
+ @group_owner = create(:user)
+ @developer = create(:user)
+ @project = create(:project, :repository,
+ creator_id: @group_owner.id,
+ star_count: 777,
+ description: 'Wow, such a cool project!')
+ @group = create(:group)
+ @group.add_user(@group_owner, GroupMember::OWNER)
+ @group.add_user(@developer, GroupMember::DEVELOPER)
+ @project.add_user(@developer, :developer)
+ @project.add_user(@group_owner, :developer)
+ @opts = { namespace: @group }
+ end
+
+ context 'fork project for group' do
+ it 'group owner successfully forks project into the group' do
+ to_project = fork_project(@project, @group_owner, @opts)
+
+ expect(to_project).to be_persisted
+ expect(to_project.errors).to be_empty
+ expect(to_project.owner).to eq(@group)
+ expect(to_project.namespace).to eq(@group)
+ expect(to_project.name).to eq(@project.name)
+ expect(to_project.path).to eq(@project.path)
+ expect(to_project.description).to eq(@project.description)
+ expect(to_project.star_count).to be_zero
end
+ end
- it "creates fork with private visibility levels" do
- forked_project = fork_project(@from_project, @to_user)
+ context 'fork project for group when user not owner' do
+ it 'group developer fails to fork project into the group' do
+ to_project = fork_project(@project, @developer, @opts)
+ expect(to_project.errors[:namespace]).to eq(['is not valid'])
+ end
+ end
- expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+ context 'project already exists in group' do
+ it 'fails due to validation, not transaction failure' do
+ existing_project = create(:project, :repository,
+ name: @project.name,
+ namespace: @group)
+ to_project = fork_project(@project, @group_owner, @opts)
+ expect(existing_project.persisted?).to be_truthy
+ expect(to_project.errors[:name]).to eq(['has already been taken'])
+ expect(to_project.errors[:path]).to eq(['has already been taken'])
end
end
end
end
- describe 'fork to namespace' do
- before do
- @group_owner = create(:user)
- @developer = create(:user)
- @project = create(:project, :repository,
- creator_id: @group_owner.id,
- star_count: 777,
- description: 'Wow, such a cool project!')
- @group = create(:group)
- @group.add_user(@group_owner, GroupMember::OWNER)
- @group.add_user(@developer, GroupMember::DEVELOPER)
- @project.add_user(@developer, :developer)
- @project.add_user(@group_owner, :developer)
- @opts = { namespace: @group }
+ context 'when linking fork to an existing project' do
+ let(:fork_from_project) { create(:project, :public) }
+ let(:fork_to_project) { create(:project, :public) }
+ let(:user) { create(:user) }
+
+ subject { described_class.new(fork_from_project, user) }
+
+ def forked_from_project(project)
+ project.fork_network_member&.forked_from_project
end
- context 'fork project for group' do
- it 'group owner successfully forks project into the group' do
- to_project = fork_project(@project, @group_owner, @opts)
-
- expect(to_project).to be_persisted
- expect(to_project.errors).to be_empty
- expect(to_project.owner).to eq(@group)
- expect(to_project.namespace).to eq(@group)
- expect(to_project.name).to eq(@project.name)
- expect(to_project.path).to eq(@project.path)
- expect(to_project.description).to eq(@project.description)
- expect(to_project.star_count).to be_zero
+ context 'if project is already forked' do
+ it 'does not create fork relation' do
+ allow(fork_to_project).to receive(:forked?).and_return(true)
+ expect(forked_from_project(fork_to_project)).to be_nil
+ expect(subject.execute(fork_to_project)).to be_nil
+ expect(forked_from_project(fork_to_project)).to be_nil
end
end
- context 'fork project for group when user not owner' do
- it 'group developer fails to fork project into the group' do
- to_project = fork_project(@project, @developer, @opts)
- expect(to_project.errors[:namespace]).to eq(['is not valid'])
+ context 'if project is not forked' do
+ it 'creates fork relation' do
+ expect(fork_to_project.forked?).to be false
+ expect(forked_from_project(fork_to_project)).to be_nil
+
+ subject.execute(fork_to_project)
+
+ expect(fork_to_project.forked?).to be true
+ expect(forked_from_project(fork_to_project)).to eq fork_from_project
+ expect(fork_to_project.forked_from_project).to eq fork_from_project
end
- end
- context 'project already exists in group' do
- it 'fails due to validation, not transaction failure' do
- existing_project = create(:project, :repository,
- name: @project.name,
- namespace: @group)
- to_project = fork_project(@project, @group_owner, @opts)
- expect(existing_project.persisted?).to be_truthy
- expect(to_project.errors[:name]).to eq(['has already been taken'])
- expect(to_project.errors[:path]).to eq(['has already been taken'])
+ it 'flushes the forks count cache of the source project' do
+ expect(fork_from_project.forks_count).to be_zero
+
+ subject.execute(fork_to_project)
+
+ expect(fork_from_project.forks_count).to eq(1)
end
end
end