From 6a33281f847fadcf0b234da2b111a2827f192208 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 3 Nov 2017 15:33:53 +0000 Subject: Merge branch 'bvl-unlink-fixes' into 'master' This fixes some bugs related to forked projects of which the source was deleted. Closes #39667 See merge request gitlab-org/gitlab-ce!15150 --- app/controllers/concerns/lfs_request.rb | 7 ++- app/helpers/projects_helper.rb | 14 ++++-- app/models/fork_network.rb | 4 ++ app/models/project.rb | 4 ++ app/services/projects/unlink_fork_service.rb | 16 ++++--- app/views/projects/_home_panel.html.haml | 9 ++-- app/views/projects/edit.html.haml | 5 ++- changelogs/unreleased/bvl-unlink-fixes.yml | 5 +++ spec/controllers/concerns/lfs_request_spec.rb | 50 ++++++++++++++++++++++ .../settings/forked_project_settings_spec.rb | 40 +++++++++++++++++ spec/models/fork_network_spec.rb | 10 +++++ spec/models/project_spec.rb | 14 ++++++ spec/services/projects/unlink_fork_service_spec.rb | 18 ++++++++ spec/support/stub_configuration.rb | 4 ++ 14 files changed, 182 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/bvl-unlink-fixes.yml create mode 100644 spec/controllers/concerns/lfs_request_spec.rb create mode 100644 spec/features/projects/settings/forked_project_settings_spec.rb diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 2b6afaa6233..738afd612f0 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -94,10 +94,9 @@ module LfsRequest @storage_project ||= begin result = project - loop do - break unless result.forked? - result = result.forked_from_project - end + # TODO: Make this go to the fork_network root immeadiatly + # dependant on the discussion in: https://gitlab.com/gitlab-org/gitlab-ce/issues/39769 + result = result.fork_source while result.forked? result end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 20e050195ea..d21c01cc27f 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -97,7 +97,15 @@ module ProjectsHelper def remove_fork_project_message(project) _("You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?") % - { forked_from_project: @project.forked_from_project.name_with_namespace } + { forked_from_project: fork_source_name(project) } + end + + def fork_source_name(project) + if @project.fork_source + @project.fork_source.full_name + else + @project.fork_network&.deleted_root_project_name + end end def project_nav_tabs @@ -127,8 +135,8 @@ module ProjectsHelper def can_change_visibility_level?(project, current_user) return false unless can?(current_user, :change_visibility_level, project) - if project.forked? - project.forked_from_project.visibility_level > Gitlab::VisibilityLevel::PRIVATE + if project.fork_source + project.fork_source.visibility_level > Gitlab::VisibilityLevel::PRIVATE else true end diff --git a/app/models/fork_network.rb b/app/models/fork_network.rb index 218e37a5312..7f1728e8c77 100644 --- a/app/models/fork_network.rb +++ b/app/models/fork_network.rb @@ -12,4 +12,8 @@ class ForkNetwork < ActiveRecord::Base def find_forks_in(other_projects) projects.where(id: other_projects) end + + def merge_requests + MergeRequest.where(target_project: projects) + end end diff --git a/app/models/project.rb b/app/models/project.rb index 984ef015f4a..4ee6db9211f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1017,6 +1017,10 @@ class Project < ActiveRecord::Base !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) end + def fork_source + forked_from_project || fork_network&.root_project + end + def personal? !group end diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index 2b82e5732e4..c499f384426 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -3,18 +3,24 @@ module Projects def execute return unless @project.forked? - @project.forked_from_project.lfs_objects.find_each do |lfs_object| - lfs_object.projects << @project + if fork_source = @project.fork_source + fork_source.lfs_objects.find_each do |lfs_object| + lfs_object.projects << @project + end + + refresh_forks_count(fork_source) end - merge_requests = @project.forked_from_project.merge_requests.opened.from_project(@project) + merge_requests = @project.fork_network + .merge_requests + .opened + .where.not(target_project: @project) + .from_project(@project) merge_requests.each do |mr| ::MergeRequests::CloseService.new(@project, @current_user).execute(mr) end - refresh_forks_count(@project.forked_from_project) - @project.fork_network_member.destroy @project.forked_project_link.destroy end diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 619b632918e..1d644dda177 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -1,6 +1,5 @@ - empty_repo = @project.empty_repo? - fork_network = @project.fork_network -- forked_from_project = @project.forked_from_project || fork_network&.root_project .project-home-panel.text-center{ class: ("empty-project" if empty_repo) } .limit-container-width{ class: container_class } .avatar-container.s70.project-avatar @@ -16,13 +15,13 @@ - if @project.forked? %p - - if forked_from_project + - if @project.fork_source #{ s_('ForkedFromProjectPath|Forked from') } - = link_to project_path(forked_from_project) do - = forked_from_project.full_name + = link_to project_path(@project.fork_source) do + = fork_source_name(@project) - else - deleted_message = s_('ForkedFromProjectPath|Forked from %{project_name} (deleted)') - = deleted_message % { project_name: fork_network.deleted_root_project_name } + = deleted_message % { project_name: fork_source_name(@project) } .project-repo-buttons .count-buttons diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 5703ef1d4bb..5ebeae5c35f 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -173,7 +173,10 @@ %p This will remove the fork relationship to source project = succeed "." do - = link_to @project.forked_from_project.name_with_namespace, project_path(@project.forked_from_project) + - if @project.fork_source + = link_to(fork_source_name(@project), project_path(@project.fork_source)) + - else + = fork_source_name(@project) = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_project_path(@project), method: :delete, remote: true, html: { class: 'transfer-project' }) do |f| %p %strong Once removed, the fork relationship cannot be restored and you will no longer be able to send merge requests to the source. diff --git a/changelogs/unreleased/bvl-unlink-fixes.yml b/changelogs/unreleased/bvl-unlink-fixes.yml new file mode 100644 index 00000000000..685d78f479d --- /dev/null +++ b/changelogs/unreleased/bvl-unlink-fixes.yml @@ -0,0 +1,5 @@ +--- +title: Fix issues with forked projects of which the source was deleted +merge_request: 15150 +author: +type: fixed diff --git a/spec/controllers/concerns/lfs_request_spec.rb b/spec/controllers/concerns/lfs_request_spec.rb new file mode 100644 index 00000000000..33b23db302a --- /dev/null +++ b/spec/controllers/concerns/lfs_request_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe LfsRequest do + include ProjectForksHelper + + controller(Projects::GitHttpClientController) do + # `described_class` is not available in this context + include LfsRequest # rubocop:disable RSpec/DescribedClass + + def show + storage_project + + render nothing: true + end + + def project + @project ||= Project.find(params[:id]) + end + + def download_request? + true + end + + def ci? + false + end + end + + let(:project) { create(:project, :public) } + + before do + stub_lfs_setting(enabled: true) + end + + describe '#storage_project' do + it 'assigns the project as storage project' do + get :show, id: project.id + + expect(assigns(:storage_project)).to eq(project) + end + + it 'assigns the source of a forked project' do + forked_project = fork_project(project) + + get :show, id: forked_project.id + + expect(assigns(:storage_project)).to eq(project) + end + end +end diff --git a/spec/features/projects/settings/forked_project_settings_spec.rb b/spec/features/projects/settings/forked_project_settings_spec.rb new file mode 100644 index 00000000000..28954a4fb40 --- /dev/null +++ b/spec/features/projects/settings/forked_project_settings_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +feature 'Settings for a forked project', :js do + include ProjectForksHelper + let(:user) { create(:user) } + let(:original_project) { create(:project) } + let(:forked_project) { fork_project(original_project, user) } + + before do + original_project.add_master(user) + forked_project.add_master(user) + sign_in(user) + end + + shared_examples 'project settings for a forked projects' do + it 'allows deleting the link to the forked project' do + visit edit_project_path(forked_project) + + click_button 'Remove fork relationship' + + wait_for_requests + + fill_in('confirm_name_input', with: forked_project.name) + click_button('Confirm') + + expect(page).to have_content('The fork relationship has been removed.') + expect(forked_project.reload.forked?).to be_falsy + end + end + + it_behaves_like 'project settings for a forked projects' + + context 'when the original project is deleted' do + before do + original_project.destroy! + end + + it_behaves_like 'project settings for a forked projects' + end +end diff --git a/spec/models/fork_network_spec.rb b/spec/models/fork_network_spec.rb index 605ccd6db06..a43baf1820a 100644 --- a/spec/models/fork_network_spec.rb +++ b/spec/models/fork_network_spec.rb @@ -24,6 +24,16 @@ describe ForkNetwork do end end + describe '#merge_requests' do + it 'finds merge requests within the fork network' do + project = create(:project) + forked_project = fork_project(project) + merge_request = create(:merge_request, source_project: forked_project, target_project: project) + + expect(project.fork_network.merge_requests).to include(merge_request) + end + end + context 'for a deleted project' do it 'keeps the fork network' do project = create(:project, :public) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cf26dbfea49..83be9642437 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1907,6 +1907,20 @@ describe Project do expect(forked_project.in_fork_network_of?(other_project)).to be_falsy end end + + describe '#fork_source' do + let!(:second_fork) { fork_project(forked_project) } + + it 'returns the direct source if it exists' do + expect(second_fork.fork_source).to eq(forked_project) + end + + it 'returns the root of the fork network when the directs source was deleted' do + forked_project.destroy + + expect(second_fork.fork_source).to eq(project) + end + end end describe '#pushes_since_gc' do diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 50d3a4ec982..2bba71fef4f 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -12,6 +12,9 @@ describe Projects::UnlinkForkService do 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_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) } + let(:mr_close_service) { MergeRequests::CloseService.new(forked_project, user) } before do @@ -22,9 +25,14 @@ describe Projects::UnlinkForkService do it 'close all pending merge requests' do expect(mr_close_service).to receive(:execute).with(merge_request) + expect(mr_close_service).to receive(:execute).with(merge_request2) subject.execute end + + it 'does not close merge requests for the project being unlinked' do + expect(mr_close_service).not_to receive(:execute).with(merge_request_in_fork) + end end it 'remove fork relation' do @@ -53,4 +61,14 @@ describe Projects::UnlinkForkService do expect(source.forks_count).to be_zero end + + context 'when the original project was deleted' do + it 'does not fail when the original project is deleted' do + source = forked_project.forked_from_project + source.destroy + forked_project.reload + + expect { subject.execute }.not_to raise_error + end + end end diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index 4d448a55978..4ead78529c3 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -38,6 +38,10 @@ module StubConfiguration allow(Gitlab.config.backup).to receive_messages(to_settings(messages)) end + def stub_lfs_setting(messages) + allow(Gitlab.config.lfs).to receive_messages(to_settings(messages)) + end + def stub_storage_settings(messages) # Default storage is always required messages['default'] ||= Gitlab.config.repositories.storages.default -- cgit v1.2.1