diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-10-19 11:01:44 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-10-19 11:01:44 +0000 |
commit | ffc505d2170fff93607e37f3a634984b90549436 (patch) | |
tree | 62c9cd0a044b226dae32fcb9ad61cfb5bba8ee07 | |
parent | e2b8337f12a45219548c37287e3929e751e4704d (diff) | |
parent | f3fba178b931cd2ba67f81ee667893cb84e7d42f (diff) | |
download | gitlab-ce-ffc505d2170fff93607e37f3a634984b90549436.tar.gz |
Merge branch 'bvl-remove-forked-project-link' into 'master'
Remove ForkedProjectLink model
Closes #38883
See merge request gitlab-org/gitlab-ce!22226
34 files changed, 217 insertions, 251 deletions
diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb index b5fb5511638..23cc9ee247a 100644 --- a/app/controllers/admin/dashboard_controller.rb +++ b/app/controllers/admin/dashboard_controller.rb @@ -3,8 +3,8 @@ class Admin::DashboardController < Admin::ApplicationController include CountHelper - COUNTED_ITEMS = [Project, User, Group, ForkedProjectLink, Issue, MergeRequest, - Note, Snippet, Key, Milestone].freeze + COUNTED_ITEMS = [Project, User, Group, ForkNetworkMember, ForkNetwork, Issue, + MergeRequest, Note, Snippet, Key, Milestone].freeze # rubocop: disable CodeReuse/ActiveRecord def index diff --git a/app/helpers/count_helper.rb b/app/helpers/count_helper.rb index e16223a82c9..13839474e1f 100644 --- a/app/helpers/count_helper.rb +++ b/app/helpers/count_helper.rb @@ -8,4 +8,18 @@ module CountHelper number_with_delimiter(count) end + + # This will approximate the fork count by checking all counting all fork network + # memberships, and deducting 1 for each root of the fork network. + # This might be inacurate as the root of the fork network might have been deleted. + # + # This makes querying this information a lot more effecient and it should be + # accurate enough for the instance wide statistics + def approximate_fork_count_with_delimiters(count_data) + fork_network_count = count_data[ForkNetwork] + fork_network_member_count = count_data[ForkNetworkMember] + approximate_fork_count = fork_network_member_count - fork_network_count + + number_with_delimiter(approximate_fork_count) + end end diff --git a/app/models/forked_project_link.rb b/app/models/forked_project_link.rb deleted file mode 100644 index 0f7067238cd..00000000000 --- a/app/models/forked_project_link.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -class ForkedProjectLink < ActiveRecord::Base - belongs_to :forked_to_project, -> { where.not(pending_delete: true) }, class_name: 'Project' - belongs_to :forked_from_project, -> { where.not(pending_delete: true) }, class_name: 'Project' -end diff --git a/app/models/project.rb b/app/models/project.rb index b80e41e4a96..086f256174f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -167,20 +167,15 @@ class Project < ActiveRecord::Base has_one :packagist_service has_one :hangouts_chat_service - # TODO: replace these relations with the fork network versions - has_one :forked_project_link, foreign_key: "forked_to_project_id" - has_one :forked_from_project, through: :forked_project_link - - has_many :forked_project_links, foreign_key: "forked_from_project_id" - has_many :forks, through: :forked_project_links, source: :forked_to_project - # TODO: replace these relations with the fork network versions - has_one :root_of_fork_network, foreign_key: 'root_project_id', inverse_of: :root_project, class_name: 'ForkNetwork' has_one :fork_network_member has_one :fork_network, through: :fork_network_member + has_one :forked_from_project, through: :fork_network_member + has_many :forked_to_members, class_name: 'ForkNetworkMember', foreign_key: 'forked_from_project_id' + has_many :forks, through: :forked_to_members, source: :project, inverse_of: :forked_from_project has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -1250,12 +1245,7 @@ class Project < ActiveRecord::Base end def forked? - return true if fork_network && fork_network.root_project != self - - # TODO: Use only the above conditional using the `fork_network` - # This is the old conditional that looks at the `forked_project_link`, we - # fall back to this while we're migrating the new models - !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) + fork_network && fork_network.root_project != self end def fork_source @@ -1546,9 +1536,7 @@ class Project < ActiveRecord::Base def visibility_level_allowed_as_fork?(level = self.visibility_level) return true unless forked? - # self.forked_from_project will be nil before the project is saved, so - # we need to go through the relation - original_project = forked_project_link&.forked_from_project + original_project = fork_source return true unless original_project level <= original_project.visibility_level diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 0e6a7e8da54..20bfe5af7a1 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -13,8 +13,8 @@ module Projects return ::Projects::CreateFromTemplateService.new(current_user, params).execute end - forked_from_project_id = params.delete(:forked_from_project_id) import_data = params.delete(:import_data) + relations_block = params.delete(:relations_block) @project = Project.new(params) @@ -24,11 +24,6 @@ module Projects return @project end - unless allowed_fork?(forked_from_project_id) - @project.errors.add(:forked_from_project_id, 'is forbidden') - return @project - end - set_project_name_from_path # get namespace id @@ -47,6 +42,7 @@ module Projects @project.namespace_id = current_user.namespace_id end + relations_block&.call(@project) yield(@project) if block_given? # If the block added errors, don't try to save the project @@ -54,10 +50,6 @@ module Projects @project.creator = current_user - if forked_from_project_id - @project.build_forked_project_link(forked_from_project_id: forked_from_project_id) - end - save_project_and_import_data(import_data) after_create_actions if @project.persisted? @@ -80,15 +72,6 @@ module Projects end # rubocop: disable CodeReuse/ActiveRecord - def allowed_fork?(source_project_id) - return true if source_project_id.nil? - - source_project = Project.find_by(id: source_project_id) - current_user.can?(:fork_project, source_project) - end - # rubocop: enable CodeReuse/ActiveRecord - - # rubocop: disable CodeReuse/ActiveRecord def allowed_namespace?(user, namespace_id) namespace = Namespace.find_by(id: namespace_id) current_user.can?(:create_projects, namespace) diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index cbbb88a9410..8dc0e044875 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -12,31 +12,42 @@ module Projects private + def allowed_fork? + current_user.can?(:fork_project, @project) + end + def link_existing_project(fork_to_project) return if fork_to_project.forked? - link_fork_network(fork_to_project) + build_fork_network_member(fork_to_project) - # A forked project stores its LFS objects in the `forked_from_project`. - # So the LFS objects become inaccessible, and therefore delete them from - # the database so they'll get cleaned up. - # - # TODO: refactor this to get the correct lfs objects when implementing - # https://gitlab.com/gitlab-org/gitlab-ce/issues/39769 - fork_to_project.lfs_objects_projects.delete_all + if link_fork_network(fork_to_project) + # A forked project stores its LFS objects in the `forked_from_project`. + # So the LFS objects become inaccessible, and therefore delete them from + # the database so they'll get cleaned up. + # + # TODO: refactor this to get the correct lfs objects when implementing + # https://gitlab.com/gitlab-org/gitlab-ce/issues/39769 + fork_to_project.lfs_objects_projects.delete_all - fork_to_project + fork_to_project + end end def fork_new_project new_params = { - forked_from_project_id: @project.id, visibility_level: allowed_visibility_level, description: @project.description, name: @project.name, path: @project.path, shared_runners_enabled: @project.shared_runners_enabled, - namespace_id: target_namespace.id + namespace_id: target_namespace.id, + fork_network: fork_network, + # We need to assign the fork network membership after the project has + # been instantiated to avoid ActiveRecord trying to create it when + # initializing the project, as that would cause a foreign key constraint + # exception. + relations_block: -> (project) { build_fork_network_member(project) } } if @project.avatar.present? && @project.avatar.image? @@ -46,38 +57,35 @@ module Projects new_project = CreateService.new(current_user, new_params).execute return new_project unless new_project.persisted? + # Set the forked_from_project relation after saving to avoid having to + # reload the project to reset the association information and cause an + # extra query. + new_project.forked_from_project = @project + builds_access_level = @project.project_feature.builds_access_level new_project.project_feature.update(builds_access_level: builds_access_level) - link_fork_network(new_project) - new_project end def fork_network - if @project.fork_network - @project.fork_network - elsif forked_from_project = @project.forked_from_project - # TODO: remove this case when all background migrations have completed - # this only happens when a project had a `forked_project_link` that was - # not migrated to the `fork_network` relation - forked_from_project.fork_network || forked_from_project.create_root_of_fork_network + @fork_network ||= @project.fork_network || @project.build_root_of_fork_network + end + + def build_fork_network_member(fork_to_project) + if allowed_fork? + fork_to_project.build_fork_network_member(forked_from_project: @project, + fork_network: fork_network) else - @project.create_root_of_fork_network + fork_to_project.errors.add(:forked_from_project_id, 'is forbidden') end end 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 + return if fork_to_project.errors.any? - refresh_forks_count + fork_to_project.fork_network_member.save && + refresh_forks_count end def refresh_forks_count diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb index 00e73148358..ca85e2dc281 100644 --- a/app/services/projects/forks_count_service.rb +++ b/app/services/projects/forks_count_service.rb @@ -9,10 +9,7 @@ module Projects # rubocop: disable CodeReuse/ActiveRecord def self.query(project_ids) - # We can't directly change ForkedProjectLink to ForkNetworkMember here - # Nowadays, when a call using v3 to projects/:id/fork is made, - # the relationship to ForkNetworkMember is not updated - ForkedProjectLink.where(forked_from_project: project_ids) + ForkNetworkMember.where(forked_from_project: project_ids) end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/services/projects/move_forks_service.rb b/app/services/projects/move_forks_service.rb index 2948555a17c..33f0bab12c9 100644 --- a/app/services/projects/move_forks_service.rb +++ b/app/services/projects/move_forks_service.rb @@ -6,7 +6,6 @@ module Projects return unless super && source_project.fork_network Project.transaction(requires_new: true) do - move_forked_project_links move_fork_network_members update_root_project refresh_forks_count @@ -18,18 +17,6 @@ module Projects private # rubocop: disable CodeReuse/ActiveRecord - def move_forked_project_links - # Update ancestor - ForkedProjectLink.where(forked_to_project: source_project) - .update_all(forked_to_project_id: @project.id) - - # Update the descendants - ForkedProjectLink.where(forked_from_project: source_project) - .update_all(forked_from_project_id: @project.id) - end - # rubocop: enable CodeReuse/ActiveRecord - - # rubocop: disable CodeReuse/ActiveRecord def move_fork_network_members ForkNetworkMember.where(project: source_project).update_all(project_id: @project.id) ForkNetworkMember.where(forked_from_project: source_project).update_all(forked_from_project_id: @project.id) diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index a8b7c7f136a..1b8a920268f 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -25,7 +25,6 @@ module Projects end @project.fork_network_member.destroy - @project.forked_project_link.destroy end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml index 85c04f8a01d..7ac79cc77f5 100644 --- a/app/views/admin/dashboard/index.html.haml +++ b/app/views/admin/dashboard/index.html.haml @@ -42,7 +42,7 @@ %p Forks %span.light.float-right - = approximate_count_with_delimiters(@counts, ForkedProjectLink) + = approximate_fork_count_with_delimiters(@counts) %p Issues %span.light.float-right diff --git a/app/workers/namespaceless_project_destroy_worker.rb b/app/workers/namespaceless_project_destroy_worker.rb index d9df42c9e17..2965f3b1150 100644 --- a/app/workers/namespaceless_project_destroy_worker.rb +++ b/app/workers/namespaceless_project_destroy_worker.rb @@ -32,7 +32,5 @@ class NamespacelessProjectDestroyWorker merge_requests = project.forked_from_project.merge_requests.opened.from_project(project) merge_requests.update_all(state: 'closed') - - project.forked_project_link.destroy end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5a4b85f98cf..65855e8aac4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -260,7 +260,7 @@ module API super(projects_relation).preload(:group) .preload(project_group_links: :group, fork_network: :root_project, - forked_project_link: :forked_from_project, + fork_network_member: :forked_from_project, forked_from_project: [:route, :forks, :tags, namespace: :route]) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 73b62dc1151..78581dc37a4 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -838,12 +838,14 @@ describe Projects::MergeRequestsController do end context 'with a forked project' do - let(:fork_project) { create(:project, :repository, forked_from_project: project) } - let(:fork_owner) { fork_project.owner } + let(:forked_project) { fork_project(project, fork_owner, repository: true) } + let(:fork_owner) { create(:user) } before do - merge_request.update!(source_project: fork_project) - fork_project.add_reporter(user) + project.add_developer(fork_owner) + + merge_request.update!(source_project: forked_project) + forked_project.add_reporter(user) end context 'user cannot push to source branch' do diff --git a/spec/factories/forked_project_links.rb b/spec/factories/forked_project_links.rb deleted file mode 100644 index bc59fea81ec..00000000000 --- a/spec/factories/forked_project_links.rb +++ /dev/null @@ -1,15 +0,0 @@ -FactoryBot.define do - factory :forked_project_link do - association :forked_to_project, factory: [:project, :repository] - association :forked_from_project, factory: [:project, :repository] - - after(:create) do |link| - link.forked_from_project.reload - link.forked_to_project.reload - end - - trait :forked_to_empty_project do - association :forked_to_project, factory: [:project, :repository] - end - end -end diff --git a/spec/features/admin/dashboard_spec.rb b/spec/features/admin/dashboard_spec.rb new file mode 100644 index 00000000000..a6ca0803469 --- /dev/null +++ b/spec/features/admin/dashboard_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe 'admin visits dashboard' do + include ProjectForksHelper + + before do + sign_in(create(:admin)) + end + + context 'counting forks' do + it 'correctly counts 2 forks of a project' do + project = create(:project) + project_fork = fork_project(project) + fork_project(project_fork) + + # Make sure the fork_networks & fork_networks reltuples have been updated + # to get a correct count on postgresql + if Gitlab::Database.postgresql? + ActiveRecord::Base.connection.execute('ANALYZE fork_networks') + ActiveRecord::Base.connection.execute('ANALYZE fork_network_members') + end + + visit admin_root_path + + expect(page).to have_content('Forks 2') + end + end +end diff --git a/spec/features/merge_request/user_edits_mr_spec.rb b/spec/features/merge_request/user_edits_mr_spec.rb index 8c9e782aa76..3152707136c 100644 --- a/spec/features/merge_request/user_edits_mr_spec.rb +++ b/spec/features/merge_request/user_edits_mr_spec.rb @@ -1,11 +1,13 @@ require 'rails_helper' describe 'Merge request > User edits MR' do + include ProjectForksHelper + it_behaves_like 'an editable merge request' context 'for a forked project' do it_behaves_like 'an editable merge request' do - let(:source_project) { create(:project, :repository, forked_from_project: target_project) } + let(:source_project) { fork_project(target_project, nil, repository: true) } end end end diff --git a/spec/features/merge_request/user_sees_mr_from_deleted_forked_project_spec.rb b/spec/features/merge_request/user_sees_mr_from_deleted_forked_project_spec.rb index 029b66b5e8e..fc4a188d4a7 100644 --- a/spec/features/merge_request/user_sees_mr_from_deleted_forked_project_spec.rb +++ b/spec/features/merge_request/user_sees_mr_from_deleted_forked_project_spec.rb @@ -1,18 +1,20 @@ require 'rails_helper' describe 'Merge request > User sees MR from deleted forked project', :js do + include ProjectForksHelper + let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } - let(:fork_project) { create(:project, :public, :repository, forked_from_project: project) } + let(:forked_project) { fork_project(project, nil, repository: true) } let!(:merge_request) do - create(:merge_request_with_diffs, source_project: fork_project, + create(:merge_request_with_diffs, source_project: forked_project, target_project: project, description: 'Test merge request') end before do MergeRequests::MergeService.new(project, user).execute(merge_request) - fork_project.destroy! + forked_project.destroy! sign_in(user) visit project_merge_request_path(project, merge_request) end diff --git a/spec/features/merge_request/user_sees_notes_from_forked_project_spec.rb b/spec/features/merge_request/user_sees_notes_from_forked_project_spec.rb index d4ad0b0a377..a6118453540 100644 --- a/spec/features/merge_request/user_sees_notes_from_forked_project_spec.rb +++ b/spec/features/merge_request/user_sees_notes_from_forked_project_spec.rb @@ -1,18 +1,20 @@ require 'rails_helper' describe 'Merge request > User sees notes from forked project', :js do + include ProjectForksHelper + let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } - let(:fork_project) { create(:project, :public, :repository, forked_from_project: project) } + let(:forked_project) { fork_project(project, nil, repository: true) } let!(:merge_request) do - create(:merge_request_with_diffs, source_project: fork_project, + create(:merge_request_with_diffs, source_project: forked_project, target_project: project, description: 'Test merge request') end before do create(:note_on_commit, note: 'A commit comment', - project: fork_project, + project: forked_project, commit_id: merge_request.commit_shas.first) sign_in(user) end diff --git a/spec/features/merge_request/user_sees_pipelines_from_forked_project_spec.rb b/spec/features/merge_request/user_sees_pipelines_from_forked_project_spec.rb index d30dcefc6aa..97093bbc2f6 100644 --- a/spec/features/merge_request/user_sees_pipelines_from_forked_project_spec.rb +++ b/spec/features/merge_request/user_sees_pipelines_from_forked_project_spec.rb @@ -1,17 +1,19 @@ require 'rails_helper' describe 'Merge request > User sees pipelines from forked project', :js do + include ProjectForksHelper + let(:target_project) { create(:project, :public, :repository) } let(:user) { target_project.creator } - let(:fork_project) { create(:project, :repository, forked_from_project: target_project) } + let(:forked_project) { fork_project(target_project, nil, repository: true) } let!(:merge_request) do - create(:merge_request_with_diffs, source_project: fork_project, + create(:merge_request_with_diffs, source_project: forked_project, target_project: target_project, description: 'Test merge request') end let(:pipeline) do create(:ci_pipeline, - project: fork_project, + project: forked_project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) end diff --git a/spec/finders/fork_projects_finder_spec.rb b/spec/finders/fork_projects_finder_spec.rb index f0cef7ea406..b3fdffc3331 100644 --- a/spec/finders/fork_projects_finder_spec.rb +++ b/spec/finders/fork_projects_finder_spec.rb @@ -1,20 +1,21 @@ require 'spec_helper' describe ForkProjectsFinder do - let(:source_project) { create(:project, :empty_repo) } - let(:private_fork) { create(:project, :private, :empty_repo, name: 'A') } - let(:internal_fork) { create(:project, :internal, :empty_repo, name: 'B') } - let(:public_fork) { create(:project, :public, :empty_repo, name: 'C') } + include ProjectForksHelper + + let(:source_project) { create(:project, :public, :empty_repo) } + let(:private_fork) { fork_project(source_project, nil, name: 'A') } + let(:internal_fork) { fork_project(source_project, nil, name: 'B') } + let(:public_fork) { fork_project(source_project, nil, name: 'C') } let(:non_member) { create(:user) } let(:private_fork_member) { create(:user) } before do + private_fork.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) private_fork.add_developer(private_fork_member) - source_project.forks << private_fork - source_project.forks << internal_fork - source_project.forks << public_fork + internal_fork.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) end describe '#execute' do diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index a3be222b7bd..e565ac8c530 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe VisibilityLevelHelper do + include ProjectForksHelper + let(:project) { build(:project) } let(:group) { build(:group) } let(:personal_snippet) { build(:personal_snippet) } @@ -83,13 +85,13 @@ describe VisibilityLevelHelper do describe "disallowed_visibility_level?" do describe "forks" do - let(:project) { create(:project, :internal) } - let(:fork_project) { create(:project, forked_from_project: project) } + let(:project) { create(:project, :internal) } + let(:forked_project) { fork_project(project) } it "disallows levels" do - expect(disallowed_visibility_level?(fork_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy - expect(disallowed_visibility_level?(fork_project, Gitlab::VisibilityLevel::INTERNAL)).to be_falsey - expect(disallowed_visibility_level?(fork_project, Gitlab::VisibilityLevel::PRIVATE)).to be_falsey + expect(disallowed_visibility_level?(forked_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy + expect(disallowed_visibility_level?(forked_project, Gitlab::VisibilityLevel::INTERNAL)).to be_falsey + expect(disallowed_visibility_level?(forked_project, Gitlab::VisibilityLevel::PRIVATE)).to be_falsey end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index fe167033941..a63f34b5536 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -229,9 +229,8 @@ project: - mock_ci_service - mock_deployment_service - mock_monitoring_service -- forked_project_link +- forked_to_members - forked_from_project -- forked_project_links - forks - merge_requests - fork_merge_requests diff --git a/spec/models/forked_project_link_spec.rb b/spec/models/forked_project_link_spec.rb deleted file mode 100644 index 32e33e8f42f..00000000000 --- a/spec/models/forked_project_link_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -require 'spec_helper' - -describe ForkedProjectLink, "add link on fork" do - include ProjectForksHelper - - let(:project_from) { create(:project, :repository) } - let(:project_to) { fork_project(project_from, user) } - let(:user) { create(:user) } - - before do - project_from.add_reporter(user) - end - - it 'project_from knows its forks' do - _ = project_to - - expect(project_from.forks.count).to eq(1) - end - - it "project_to knows it is forked" do - expect(project_to.forked?).to be_truthy - end - - it "project knows who it is forked from" do - expect(project_to.forked_from_project).to eq(project_from) - end - - context 'project_to is pending_delete' do - before do - project_to.update!(pending_delete: true) - end - - it { expect(project_from.forks.count).to eq(0) } - end - - context 'project_from is pending_delete' do - before do - project_from.update!(pending_delete: true) - end - - it { expect(project_to.forked_from_project).to be_nil } - end - - describe '#forked?' do - let(:project_to) { create(:project, :repository, forked_project_link: forked_project_link) } - let(:forked_project_link) { create(:forked_project_link) } - - before do - forked_project_link.forked_from_project = project_from - forked_project_link.forked_to_project = project_to - forked_project_link.save! - end - - it "project_to knows it is forked" do - expect(project_to.forked?).to be_truthy - end - - it "project_from is not forked" do - expect(project_from.forked?).to be_falsey - end - - it "project_to.destroy destroys fork_link" do - project_to.destroy - - expect(ForkedProjectLink.exists?(id: forked_project_link.id)).to eq(false) - end - end -end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 88f70a88210..b42c7c6e62d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -23,7 +23,6 @@ describe Project do it { is_expected.to have_many(:deploy_keys) } it { is_expected.to have_many(:hooks) } it { is_expected.to have_many(:protected_branches) } - it { is_expected.to have_one(:forked_project_link) } it { is_expected.to have_one(:slack_service) } it { is_expected.to have_one(:microsoft_teams_service) } it { is_expected.to have_one(:mattermost_service) } @@ -56,7 +55,7 @@ describe Project do it { is_expected.to have_one(:statistics).class_name('ProjectStatistics') } it { is_expected.to have_one(:import_data).class_name('ProjectImportData') } it { is_expected.to have_one(:last_event).class_name('Event') } - it { is_expected.to have_one(:forked_from_project).through(:forked_project_link) } + it { is_expected.to have_one(:forked_from_project).through(:fork_network_member) } it { is_expected.to have_one(:auto_devops).class_name('ProjectAutoDevops') } it { is_expected.to have_many(:commit_statuses) } it { is_expected.to have_many(:pipelines) } @@ -77,7 +76,8 @@ describe Project do it { is_expected.to have_many(:lfs_objects_projects) } it { is_expected.to have_many(:project_group_links) } it { is_expected.to have_many(:notification_settings).dependent(:delete_all) } - it { is_expected.to have_many(:forks).through(:forked_project_links) } + it { is_expected.to have_many(:forked_to_members).class_name('ForkNetworkMember') } + it { is_expected.to have_many(:forks).through(:forked_to_members) } it { is_expected.to have_many(:uploads) } it { is_expected.to have_many(:pipeline_schedules) } it { is_expected.to have_many(:members_and_requesters) } @@ -1380,7 +1380,7 @@ describe Project do context 'when checking on forked project' do let(:project) { create(:project, :internal) } - let(:forked_project) { create(:project, forked_from_project: project) } + let(:forked_project) { fork_project(project) } it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy } it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy } @@ -1992,9 +1992,12 @@ describe Project do let(:import_jid) { '123' } context 'forked' do - let(:forked_project_link) { create(:forked_project_link, :forked_to_empty_project) } - let(:forked_from_project) { forked_project_link.forked_from_project } - let(:project) { forked_project_link.forked_to_project } + let(:forked_from_project) { create(:project, :repository) } + let(:project) { create(:project) } + + before do + fork_project(forked_from_project, nil, target_project: project) + end it 'schedules a RepositoryForkWorker job' do expect(RepositoryForkWorker).to receive(:perform_async).with(project.id).and_return(import_jid) @@ -2281,6 +2284,12 @@ describe Project do end end + describe '#forks' do + it 'includes direct forks of the project' do + expect(project.forks).to contain_exactly(forked_project) + end + end + describe '#lfs_storage_project' do it 'returns self for non-forks' do expect(project.lfs_storage_project).to eq project diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index c8e98e6024c..22e5a7c7174 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1196,7 +1196,7 @@ describe API::Projects do expect(response).to have_gitlab_http_status(201) expect(project_fork_target.forked_from_project.id).to eq(project_fork_source.id) - expect(project_fork_target.forked_project_link).to be_present + expect(project_fork_target.fork_network_member).to be_present expect(project_fork_target).to be_forked end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 0ba2539a717..5bf8aa7f23f 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe MergeRequestWidgetEntity do + include ProjectForksHelper + let(:project) { create :project, :repository } let(:resource) { create(:merge_request, source_project: project, target_project: project) } let(:user) { create(:user) } @@ -206,12 +208,12 @@ describe MergeRequestWidgetEntity do describe 'when source project is deleted' do let(:project) { create(:project, :repository) } - let(:fork_project) { create(:project, :repository, forked_from_project: project) } - let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: project) } + let(:forked_project) { fork_project(project) } + let(:merge_request) { create(:merge_request, source_project: forked_project, target_project: project) } it 'returns a blank rebase_path' do allow(merge_request).to receive(:should_be_rebased?).and_return(true) - fork_project.destroy + forked_project.destroy merge_request.reload entity = described_class.new(merge_request, request: request).as_json diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index a80c8a7fe51..08de27ca44a 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -295,6 +295,17 @@ describe Projects::CreateService, '#execute' do end end + it 'calls the passed block' do + fake_block = double('block') + opts[:relations_block] = fake_block + + expect_next_instance_of(Project) do |project| + expect(fake_block).to receive(:call).with(project) + end + + create_project(user, opts) + end + it 'writes project full path to .git/config' do project = create_project(user, opts) rugged = rugged_repo(project.repository) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 1d31d26f418..12ddf8447bd 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -259,7 +259,6 @@ describe Projects::DestroyService do before do project.lfs_objects << create(:lfs_object) - forked_project.forked_project_link.destroy forked_project.reload end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 947cb61038d..a3d24ae312a 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -31,6 +31,10 @@ describe Projects::ForkService do it { is_expected.not_to be_persisted } it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) } + + it 'does not create a fork network' do + expect { subject }.not_to change { @from_project.reload.fork_network } + end end describe "successfully creates project in the user namespace" do @@ -70,6 +74,12 @@ describe Projects::ForkService do expect(fork_network.root_project).to eq(@from_project) expect(fork_network.projects).to contain_exactly(@from_project, to_project) end + + it 'imports the repository of the forked project' do + to_project = fork_project(@from_project, @to_user, repository: true) + + expect(to_project.empty_repo?).to be_falsy + end end context 'creating a fork of a fork' do @@ -247,11 +257,13 @@ describe Projects::ForkService do context 'if project is not forked' do it 'creates fork relation' do - expect(fork_to_project.forked?).to be false + expect(fork_to_project.forked?).to be_falsy expect(forked_from_project(fork_to_project)).to be_nil subject.execute(fork_to_project) + fork_to_project.reload + 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 @@ -272,6 +284,17 @@ describe Projects::ForkService do .to change { fork_to_project.lfs_objects_projects.count } .to(0) end + + context 'if the fork is not allowed' do + let(:fork_from_project) { create(:project, :private) } + + it 'does not delete the LFS objects' do + create(:lfs_objects_project, project: fork_to_project) + + expect { subject.execute(fork_to_project) } + .not_to change { fork_to_project.lfs_objects_projects.size } + end + end end end end diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 3ec6139bfa6..014aab44281 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -5,13 +5,12 @@ describe Projects::UnlinkForkService do subject { described_class.new(forked_project, user) } - let(:fork_link) { forked_project.forked_project_link } let(:project) { create(:project, :public) } let(:forked_project) { fork_project(project, user) } let(:user) { create(:user) } context 'with opened merge request on the source project' do - let(:merge_request) { create(:merge_request, source_project: forked_project, target_project: fork_link.forked_from_project) } + let(:merge_request) { create(:merge_request, source_project: forked_project, target_project: forked_project.forked_from_project) } let(:merge_request2) { create(:merge_request, source_project: forked_project, target_project: fork_project(project)) } let(:merge_request_in_fork) { create(:merge_request, source_project: forked_project, target_project: forked_project) } @@ -35,12 +34,6 @@ describe Projects::UnlinkForkService do end end - it 'remove fork relation' do - expect(forked_project.forked_project_link).to receive(:destroy) - - subject.execute - end - it 'removes the link to the fork network' do expect(forked_project.fork_network_member).to be_present expect(forked_project.fork_network).to be_present diff --git a/spec/support/helpers/project_forks_helper.rb b/spec/support/helpers/project_forks_helper.rb index 2c501a2a27c..6a7132c3093 100644 --- a/spec/support/helpers/project_forks_helper.rb +++ b/spec/support/helpers/project_forks_helper.rb @@ -24,7 +24,7 @@ module ProjectForksHelper allow(service).to receive(:gitlab_shell).and_return(shell) end - forked_project = service.execute + forked_project = service.execute(params[:target_project]) # Reload the both projects so they know about their newly created fork_network if forked_project.persisted? diff --git a/spec/workers/namespaceless_project_destroy_worker_spec.rb b/spec/workers/namespaceless_project_destroy_worker_spec.rb index eec110dfbfb..2f21a1321e1 100644 --- a/spec/workers/namespaceless_project_destroy_worker_spec.rb +++ b/spec/workers/namespaceless_project_destroy_worker_spec.rb @@ -71,10 +71,10 @@ describe NamespacelessProjectDestroyWorker do expect(merge_request.reload).to be_closed end - it 'destroys the link' do + it 'destroys fork network members' do subject.perform(project.id) - expect(parent_project.forked_project_links).to be_empty + expect(parent_project.forked_to_members).to be_empty end end end diff --git a/spec/workers/rebase_worker_spec.rb b/spec/workers/rebase_worker_spec.rb index 20aff020dbb..936b9deaecc 100644 --- a/spec/workers/rebase_worker_spec.rb +++ b/spec/workers/rebase_worker_spec.rb @@ -1,25 +1,23 @@ require 'spec_helper' describe RebaseWorker, '#perform' do + include ProjectForksHelper + context 'when rebasing an MR from a fork where upstream has protected branches' do let(:upstream_project) { create(:project, :repository) } - let(:fork_project) { create(:project, :repository) } + let(:forked_project) { fork_project(upstream_project, nil, repository: true) } let(:merge_request) do create(:merge_request, - source_project: fork_project, + source_project: forked_project, source_branch: 'feature_conflict', target_project: upstream_project, target_branch: 'master') end - before do - create(:forked_project_link, forked_to_project: fork_project, forked_from_project: upstream_project) - end - it 'sets the correct project for running hooks' do expect(MergeRequests::RebaseService) - .to receive(:new).with(fork_project, merge_request.author).and_call_original + .to receive(:new).with(forked_project, merge_request.author).and_call_original subject.perform(merge_request, merge_request.author) end diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb index ac8716ecfb1..781f91ac9ca 100644 --- a/spec/workers/repository_fork_worker_spec.rb +++ b/spec/workers/repository_fork_worker_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe RepositoryForkWorker do + include ProjectForksHelper + describe 'modules' do it 'includes ProjectImportOptions' do expect(described_class).to include_module(ProjectImportOptions) @@ -8,9 +10,13 @@ describe RepositoryForkWorker do end describe "#perform" do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :public, :repository) } let(:shell) { Gitlab::Shell.new } - let(:fork_project) { create(:project, :repository, :import_scheduled, forked_from_project: project) } + let(:forked_project) { create(:project, :repository, :import_scheduled) } + + before do + fork_project(project, forked_project.creator, target_project: forked_project, repository: true) + end shared_examples 'RepositoryForkWorker performing' do before do @@ -21,8 +27,8 @@ describe RepositoryForkWorker do expect(shell).to receive(:fork_repository).with( 'default', project.disk_path, - fork_project.repository_storage, - fork_project.disk_path + forked_project.repository_storage, + forked_project.disk_path ) end @@ -49,28 +55,28 @@ describe RepositoryForkWorker do perform! - expect(fork_project.protected_branches.first.name).to eq(fork_project.default_branch) + expect(forked_project.protected_branches.first.name).to eq(forked_project.default_branch) end it 'flushes various caches' do expect_fork_repository.and_return(true) # Works around https://github.com/rspec/rspec-mocks/issues/910 - expect(Project).to receive(:find).with(fork_project.id).and_return(fork_project) - expect(fork_project.repository).to receive(:expire_emptiness_caches) + expect(Project).to receive(:find).with(forked_project.id).and_return(forked_project) + expect(forked_project.repository).to receive(:expire_emptiness_caches) .and_call_original - expect(fork_project.repository).to receive(:expire_exists_cache) + expect(forked_project.repository).to receive(:expire_exists_cache) .and_call_original - expect(fork_project.wiki.repository).to receive(:expire_emptiness_caches) + expect(forked_project.wiki.repository).to receive(:expire_emptiness_caches) .and_call_original - expect(fork_project.wiki.repository).to receive(:expire_exists_cache) + expect(forked_project.wiki.repository).to receive(:expire_exists_cache) .and_call_original perform! end it "handles bad fork" do - error_message = "Unable to fork project #{fork_project.id} for repository #{project.disk_path} -> #{fork_project.disk_path}" + error_message = "Unable to fork project #{forked_project.id} for repository #{project.disk_path} -> #{forked_project.disk_path}" expect_fork_repository.and_return(false) @@ -80,7 +86,7 @@ describe RepositoryForkWorker do context 'only project ID passed' do def perform! - subject.perform(fork_project.id) + subject.perform(forked_project.id) end it_behaves_like 'RepositoryForkWorker performing' @@ -88,7 +94,7 @@ describe RepositoryForkWorker do context 'project ID, storage and repo paths passed' do def perform! - subject.perform(fork_project.id, TestEnv.repos_path, project.disk_path) + subject.perform(forked_project.id, TestEnv.repos_path, project.disk_path) end it_behaves_like 'RepositoryForkWorker performing' |