From 68b5ac7f185b9b30cd862eacf806db726d8ce6e4 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Tue, 7 Oct 2014 14:55:15 -0500 Subject: Add option to keep repo on project delete Update the project API controller to use `Projects::DestroyService` instead of calling `Project#destroy` directly. Also add an optional parameter, `:keep_repo`, that allows a project to be deleted without deleting the repository, wiki, and satellite from disk. --- app/controllers/projects_controller.rb | 3 ++- app/services/projects/destroy_service.rb | 13 ++++++++----- lib/api/projects.rb | 8 +++++++- spec/features/projects_spec.rb | 7 ++++++- spec/requests/api/projects_spec.rb | 15 +++++++++++++++ 5 files changed, 38 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b3380a6ff23..c881c921ce9 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -100,7 +100,8 @@ class ProjectsController < ApplicationController def destroy return access_denied! unless can?(current_user, :remove_project, project) - ::Projects::DestroyService.new(@project, current_user, {}).execute + ::Projects::DestroyService.new(@project, current_user, + keep_repo: params[:keep_repo]).execute respond_to do |format| format.html do diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 7e1d753b021..7c7892a0b14 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -6,7 +6,10 @@ module Projects project.team.truncate project.repository.expire_cache unless project.empty_repo? - if project.destroy + result = project.destroy + return false unless result + + unless params[:keep_repo] GitlabShellWorker.perform_async( :remove_repository, project.path_with_namespace @@ -18,11 +21,11 @@ module Projects ) project.satellite.destroy - - log_info("Project \"#{project.name}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - true end + + log_info("Project \"#{project.name}\" was removed") + system_hook_service.execute_hooks_for(project, :destroy) + result end end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 7f7d2f8e9a8..e70548d1e85 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -174,11 +174,17 @@ module API # # Parameters: # id (required) - The ID of a project + # keep_repo (optional) - If true, then delete the project from the + # database but keep the repo, wiki, and satellite on disk. # Example Request: # DELETE /projects/:id delete ":id" do authorize! :remove_project, user_project - user_project.destroy + ::Projects::DestroyService.new( + user_project, + current_user, + keep_repo: params[:keep_repo] + ).execute end # Mark this project as forked from another diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 524c4d5fa21..369db56d493 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -10,7 +10,12 @@ describe "Projects", feature: true do visit edit_project_path(@project) end - it "should be correct path" do + it 'should delete the project from the database and disk' do + expect(GitlabShellWorker).to( + receive(:perform_async).with(:remove_repository, + /#{@project.path_with_namespace}/) + ).twice + expect { click_link "Remove project" }.to change {Project.count}.by(-1) end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index aa1437c71aa..6de37cff0aa 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -632,10 +632,25 @@ describe API::API, api: true do describe "DELETE /projects/:id" do context "when authenticated as user" do it "should remove project" do + expect(GitlabShellWorker).to( + receive(:perform_async).with(:remove_repository, + /#{project.path_with_namespace}/) + ).twice + delete api("/projects/#{project.id}", user) response.status.should == 200 end + it 'should keep repo when "keep_repo" param is true' do + expect(GitlabShellWorker).not_to( + receive(:perform_async).with(:remove_repository, + /#{project.path_with_namespace}/) + ) + + delete api("/projects/#{project.id}?keep_repo=true", user) + response.status.should == 200 + end + it "should not remove a project if not an owner" do user3 = create(:user) project.team << [user3, :developer] -- cgit v1.2.1 From 41518a467dcef61deca24ad2f6205c6fd5706e1b Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Wed, 22 Oct 2014 21:08:19 -0500 Subject: Remove :keep_repo option Always delete repositories from the filesystem when deleting a project. --- app/controllers/projects_controller.rb | 3 +-- app/services/projects/destroy_service.rb | 13 +++++-------- lib/api/projects.rb | 8 +------- spec/requests/api/projects_spec.rb | 10 ---------- 4 files changed, 7 insertions(+), 27 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index c881c921ce9..b3380a6ff23 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -100,8 +100,7 @@ class ProjectsController < ApplicationController def destroy return access_denied! unless can?(current_user, :remove_project, project) - ::Projects::DestroyService.new(@project, current_user, - keep_repo: params[:keep_repo]).execute + ::Projects::DestroyService.new(@project, current_user, {}).execute respond_to do |format| format.html do diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 7c7892a0b14..7e1d753b021 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -6,10 +6,7 @@ module Projects project.team.truncate project.repository.expire_cache unless project.empty_repo? - result = project.destroy - return false unless result - - unless params[:keep_repo] + if project.destroy GitlabShellWorker.perform_async( :remove_repository, project.path_with_namespace @@ -21,11 +18,11 @@ module Projects ) project.satellite.destroy - end - log_info("Project \"#{project.name}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - result + log_info("Project \"#{project.name}\" was removed") + system_hook_service.execute_hooks_for(project, :destroy) + true + end end end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index e70548d1e85..7fcf97d1ad6 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -174,17 +174,11 @@ module API # # Parameters: # id (required) - The ID of a project - # keep_repo (optional) - If true, then delete the project from the - # database but keep the repo, wiki, and satellite on disk. # Example Request: # DELETE /projects/:id delete ":id" do authorize! :remove_project, user_project - ::Projects::DestroyService.new( - user_project, - current_user, - keep_repo: params[:keep_repo] - ).execute + ::Projects::DestroyService.new(user_project, current_user, {}).execute end # Mark this project as forked from another diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 6de37cff0aa..ba7ec7b2be9 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -641,16 +641,6 @@ describe API::API, api: true do response.status.should == 200 end - it 'should keep repo when "keep_repo" param is true' do - expect(GitlabShellWorker).not_to( - receive(:perform_async).with(:remove_repository, - /#{project.path_with_namespace}/) - ) - - delete api("/projects/#{project.id}?keep_repo=true", user) - response.status.should == 200 - end - it "should not remove a project if not an owner" do user3 = create(:user) project.team << [user3, :developer] -- cgit v1.2.1