From c44764f523cea756f1f2efdc4db954f4f19df440 Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Fri, 3 Oct 2014 10:12:44 +0200 Subject: [PATCH 1/5] Prepare ForkService to support forking projects to given namespaces Remove overload of BaseService.initialize, so initialize gains params, which is used to pass the namespace (like e.g. in TransferService). The namespace is checked for permission to create projects in it. --- CHANGELOG | 1 + app/services/projects/fork_service.rb | 19 +++++--- spec/services/projects/fork_service_spec.rb | 52 +++++++++++++++++++-- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 0250b4a23c0..410863d3f99 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -9,6 +9,7 @@ v 7.4.0 - Do not delete tmp/repositories itself during clean-up, only its contents - Support for backup uploads to remote storage - Prevent notes polling when there are not notes + - Internal ForkService: Prepare support for fork to a given namespace - API: Add support for forking a project via the API (Bernhard Kaindl) - API: filter project issues by milestone (Julien Bianchi) - Fail harder in the backup script diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index a59311bf942..c4f2d08efe9 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -2,11 +2,9 @@ module Projects class ForkService < BaseService include Gitlab::ShellAdapter - def initialize(project, user) - @from_project, @current_user = project, user - end - def execute + @from_project = @project + project_params = { visibility_level: @from_project.visibility_level, description: @from_project.description, @@ -15,8 +13,15 @@ module Projects project = Project.new(project_params) project.name = @from_project.name project.path = @from_project.path - project.namespace = current_user.namespace - project.creator = current_user + project.namespace = @current_user.namespace + if namespace = @params[:namespace] + project.namespace = namespace + end + project.creator = @current_user + unless @current_user.can?(:create_projects, project.namespace) + project.errors.add(:namespace, 'insufficient access rights') + return project + end # If the project cannot save, we do not want to trigger the project destroy # as this can have the side effect of deleting a repo attached to an existing @@ -27,7 +32,7 @@ module Projects #First save the DB entries as they can be rolled back if the repo fork fails project.build_forked_project_link(forked_to_project_id: project.id, forked_from_project_id: @from_project.id) if project.save - project.team << [current_user, :master] + project.team << [@current_user, :master] end #Now fork the repo unless gitlab_shell.fork_repository(@from_project.path_with_namespace, project.namespace.path) diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 0edc3a8e807..5c80345c2b3 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -42,10 +42,54 @@ describe Projects::ForkService do end end - def fork_project(from_project, user, fork_success = true) - context = Projects::ForkService.new(from_project, user) - shell = double("gitlab_shell") - shell.stub(fork_repository: fork_success) + describe :fork_to_namespace do + before do + @group_owner = create(:user) + @developer = create(:user) + @project = create(:project, 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) + @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, true, @opts) + to_project.owner.should == @group + to_project.namespace.should == @group + to_project.name.should == @project.name + to_project.path.should == @project.path + to_project.description.should == @project.description + to_project.star_count.should be_zero + end + end + + context 'fork project for group when user not owner' do + it 'group developer should fail to fork project into the group' do + to_project = fork_project(@project, @developer, true, @opts) + to_project.errors[:namespace].should == ['insufficient access rights'] + end + end + + context 'project already exists in group' do + it 'should fail due to validation, not transaction failure' do + existing_project = create(:project, name: @project.name, + namespace: @group) + to_project = fork_project(@project, @group_owner, true, @opts) + existing_project.persisted?.should be_true + to_project.errors[:base].should == ['Invalid fork destination'] + to_project.errors[:name].should == ['has already been taken'] + to_project.errors[:path].should == ['has already been taken'] + end + end + end + + def fork_project(from_project, user, fork_success = true, params = {}) + context = Projects::ForkService.new(from_project, user, params) + shell = double('gitlab_shell').stub(fork_repository: fork_success) context.stub(gitlab_shell: shell) context.execute end From 18c8226566edb1c7fa43ccc1bf7a1db33f91489f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 13 Nov 2014 19:40:47 +0200 Subject: [PATCH 2/5] Refactor project fork service Signed-off-by: Dmitriy Zaporozhets --- app/services/projects/fork_service.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index c4f2d08efe9..4930660055a 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -13,11 +13,14 @@ module Projects project = Project.new(project_params) project.name = @from_project.name project.path = @from_project.path - project.namespace = @current_user.namespace + project.creator = @current_user + if namespace = @params[:namespace] project.namespace = namespace + else + project.namespace = @current_user.namespace end - project.creator = @current_user + unless @current_user.can?(:create_projects, project.namespace) project.errors.add(:namespace, 'insufficient access rights') return project @@ -47,8 +50,8 @@ module Projects else project.errors.add(:base, "Invalid fork destination") end - project + project end end end From e08e405ac4c448d8b720ed2ef6181c15e3f3dfc1 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 13 Nov 2014 22:06:19 +0200 Subject: [PATCH 3/5] Select namespace where to fork project Now you can fork project into group or personal namespace. Also I moved fork logic from ProjectsController to own fork resource Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/forks_controller.rb | 22 ++++++++++++++++++++ app/controllers/projects_controller.rb | 16 -------------- app/models/user.rb | 10 +++++++++ app/views/projects/_home_panel.html.haml | 2 +- app/views/projects/fork.html.haml | 19 ----------------- app/views/projects/forks/error.html.haml | 20 ++++++++++++++++++ app/views/projects/forks/new.html.haml | 19 +++++++++++++++++ config/routes.rb | 11 +++++----- 8 files changed, 78 insertions(+), 41 deletions(-) create mode 100644 app/controllers/projects/forks_controller.rb delete mode 100644 app/views/projects/fork.html.haml create mode 100644 app/views/projects/forks/error.html.haml create mode 100644 app/views/projects/forks/new.html.haml diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb new file mode 100644 index 00000000000..a0481d11582 --- /dev/null +++ b/app/controllers/projects/forks_controller.rb @@ -0,0 +1,22 @@ +class Projects::ForksController < Projects::ApplicationController + # Authorize + before_filter :authorize_download_code! + before_filter :require_non_empty_project + + def new + @namespaces = current_user.manageable_namespaces + @namespaces.delete(@project.namespace) + end + + def create + namespace = Namespace.find(params[:namespace_id]) + @forked_project = ::Projects::ForkService.new(project, current_user, namespace: namespace).execute + + if @forked_project.saved? && @forked_project.forked? + redirect_to(@forked_project, notice: 'Project was successfully forked.') + else + @title = 'Fork project' + render :error + end + end +end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b5910c902e4..b3181fa310e 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -111,22 +111,6 @@ class ProjectsController < ApplicationController end end - def fork - @forked_project = ::Projects::ForkService.new(project, current_user).execute - - respond_to do |format| - format.html do - if @forked_project.saved? && @forked_project.forked? - redirect_to(@forked_project, notice: 'Project was successfully forked.') - else - @title = 'Fork project' - render "fork" - end - end - format.js - end - end - def autocomplete_sources note_type = params['type'] note_id = params['type_id'] diff --git a/app/models/user.rb b/app/models/user.rb index d400edc0df5..fc191a78f53 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -551,4 +551,14 @@ class User < ActiveRecord::Base UsersStarProject.create!(project: project, user: self) end end + + def manageable_namespaces + @manageable_namespaces ||= + begin + namespaces = [] + namespaces << namespace + namespaces += owned_groups + namespaces += masters_groups + end + end end diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index 672a91e0eef..c2fa1c4bccc 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -20,7 +20,7 @@ = link_to project_path(current_user.fork_of(@project)), title: 'Go to my fork' do = link_to_toggle_fork - else - = link_to fork_project_path(@project), title: "Fork project", method: "POST" do + = link_to new_project_fork_path(@project), title: "Fork project" do = link_to_toggle_fork .star-buttons diff --git a/app/views/projects/fork.html.haml b/app/views/projects/fork.html.haml deleted file mode 100644 index d8f5c7b98d6..00000000000 --- a/app/views/projects/fork.html.haml +++ /dev/null @@ -1,19 +0,0 @@ -.alert.alert-danger.alert-block - %h4 - %i.fa.fa-code-fork - Fork Error! - %p - You tried to fork - = link_to_project @project - but it failed for the following reason: - - - - if @forked_project && @forked_project.errors.any? - %p - – - = @forked_project.errors.full_messages.first - - %p - = link_to fork_project_path(@project), title: "Fork", class: "btn", method: "POST" do - %i.fa.fa-code-fork - Try to Fork again diff --git a/app/views/projects/forks/error.html.haml b/app/views/projects/forks/error.html.haml new file mode 100644 index 00000000000..76d3aa5bf00 --- /dev/null +++ b/app/views/projects/forks/error.html.haml @@ -0,0 +1,20 @@ +- if @forked_project && !@forked_project.saved? + .alert.alert-danger.alert-block + %h4 + %i.fa.fa-code-fork + Fork Error! + %p + You tried to fork + = link_to_project @project + but it failed for the following reason: + + + - if @forked_project && @forked_project.errors.any? + %p + – + = @forked_project.errors.full_messages.first + + %p + = link_to new_project_fork_path(@project), title: "Fork", class: "btn" do + %i.fa.fa-code-fork + Try to Fork again diff --git a/app/views/projects/forks/new.html.haml b/app/views/projects/forks/new.html.haml new file mode 100644 index 00000000000..db7486b00e8 --- /dev/null +++ b/app/views/projects/forks/new.html.haml @@ -0,0 +1,19 @@ +%h3.page-title Fork project +%p.lead Select namespace where to fork this project +%hr + +- @namespaces.in_groups_of(6, false) do |group| + .row + - group.each do |namespace| + .col-md-2.col-sm-3 + .thumbnail + = link_to project_fork_path(@project, namespace_id: namespace.id), title: "Fork here", method: "POST" do + - if namespace.kind_of?(Group) + = image_tag group_icon(namespace.path) + - else + = image_tag avatar_icon(namespace.owner.email, 200) + .caption + %h4=namespace.human_name + %p + = namespace.path + diff --git a/config/routes.rb b/config/routes.rb index 2534153758b..470fe7f4dc1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -181,7 +181,6 @@ Gitlab::Application.routes.draw do resources :projects, constraints: { id: /[a-zA-Z.0-9_\-]+\/[a-zA-Z.0-9_\-]+/ }, except: [:new, :create, :index], path: "/" do member do put :transfer - post :fork post :archive post :unarchive post :upload_image @@ -214,11 +213,11 @@ Gitlab::Application.routes.draw do match "/compare/:from...:to" => "compare#show", as: "compare", via: [:get, :post], constraints: {from: /.+/, to: /.+/} - resources :snippets, constraints: {id: /\d+/} do - member do - get "raw" - end + resources :snippets, constraints: {id: /\d+/} do + member do + get "raw" end + end resources :wikis, only: [:show, :edit, :destroy, :create], constraints: {id: /[a-zA-Z.0-9_\-\/]+/} do collection do @@ -232,6 +231,8 @@ Gitlab::Application.routes.draw do end end + resource :fork, only: [:new, :create] + resource :repository, only: [:show] do member do get "archive", constraints: { format: Gitlab::Regex.archive_formats_regex } From d2c3c98e3cf88dd59a2a1a0d94e711e31c11b2cd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 14 Nov 2014 12:55:13 +0200 Subject: [PATCH 4/5] Routing specs for fork projects Signed-off-by: Dmitriy Zaporozhets --- spec/routing/project_routing_spec.rb | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 4b2eb42c709..ea584c9802d 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -55,7 +55,6 @@ end # projects POST /projects(.:format) projects#create # new_project GET /projects/new(.:format) projects#new -# fork_project POST /:id/fork(.:format) projects#fork # files_project GET /:id/files(.:format) projects#files # edit_project GET /:id/edit(.:format) projects#edit # project GET /:id(.:format) projects#show @@ -70,10 +69,6 @@ describe ProjectsController, "routing" do get("/projects/new").should route_to('projects#new') end - it "to #fork" do - post("/gitlab/gitlabhq/fork").should route_to('projects#fork', id: 'gitlab/gitlabhq') - end - it "to #edit" do get("/gitlab/gitlabhq/edit").should route_to('projects#edit', id: 'gitlab/gitlabhq') end @@ -462,3 +457,13 @@ describe Projects::GraphsController, "routing" do get("/gitlab/gitlabhq/graphs/master").should route_to('projects/graphs#show', project_id: 'gitlab/gitlabhq', id: 'master') end end + +describe Projects::ForksController, "routing" do + it "to #new" do + get("/gitlab/gitlabhq/fork/new").should route_to("projects/forks#new", project_id: 'gitlab/gitlabhq') + end + + it "to #create" do + post("/gitlab/gitlabhq/fork").should route_to("projects/forks#create", project_id: 'gitlab/gitlabhq') + end +end From 2388fdd7c6274dad8c10f5bc517f0a8b1aa28aa3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 14 Nov 2014 16:06:39 +0200 Subject: [PATCH 5/5] Improve fork to namespaces feature * Show namespace thumbnail differently if project was already forked * Show loading spinner when click on fork * Fork link navigates to personal namespace only if no manageable groups exists Signed-off-by: Dmitriy Zaporozhets --- app/assets/javascripts/dispatcher.js.coffee | 2 + app/assets/javascripts/project_fork.js.coffee | 5 ++ app/assets/stylesheets/sections/projects.scss | 25 ++++++++++ app/helpers/namespaces_helper.rb | 8 ++++ app/models/namespace.rb | 4 ++ app/views/projects/_home_panel.html.haml | 2 +- app/views/projects/forks/new.html.haml | 47 +++++++++++++------ features/project/fork.feature | 2 + features/steps/project/fork.rb | 6 +++ 9 files changed, 86 insertions(+), 15 deletions(-) create mode 100644 app/assets/javascripts/project_fork.js.coffee diff --git a/app/assets/javascripts/dispatcher.js.coffee b/app/assets/javascripts/dispatcher.js.coffee index fb1adbc4b3d..e8b71a71945 100644 --- a/app/assets/javascripts/dispatcher.js.coffee +++ b/app/assets/javascripts/dispatcher.js.coffee @@ -75,6 +75,8 @@ class Dispatcher # Ensure we don't create a particular shortcut handler here. This is # already created, where the network graph is created. shortcut_handler = true + when 'projects:forks:new' + new ProjectFork() when 'users:show' new User() diff --git a/app/assets/javascripts/project_fork.js.coffee b/app/assets/javascripts/project_fork.js.coffee new file mode 100644 index 00000000000..e15a1c4ef76 --- /dev/null +++ b/app/assets/javascripts/project_fork.js.coffee @@ -0,0 +1,5 @@ +class @ProjectFork + constructor: -> + $('.fork-thumbnail a').on 'click', -> + $('.fork-namespaces').hide() + $('.save-project-loader').show() diff --git a/app/assets/stylesheets/sections/projects.scss b/app/assets/stylesheets/sections/projects.scss index b4ee5ccc8d7..76a7507d699 100644 --- a/app/assets/stylesheets/sections/projects.scss +++ b/app/assets/stylesheets/sections/projects.scss @@ -270,3 +270,28 @@ ul.nav.nav-projects-tabs { color: #999; } } + +.fork-namespaces { + .thumbnail { + + &.fork-exists-thumbnail { + border-color: #EEE; + + .caption { + color: #999; + } + } + + &.fork-thumbnail { + border-color: #AAA; + + &:hover { + background-color: $hover; + } + } + + a { + text-decoration: none; + } + } +} diff --git a/app/helpers/namespaces_helper.rb b/app/helpers/namespaces_helper.rb index bf25dce2301..2bcfde62830 100644 --- a/app/helpers/namespaces_helper.rb +++ b/app/helpers/namespaces_helper.rb @@ -25,4 +25,12 @@ module NamespacesHelper hidden_field_tag(id, value, class: css_class) end + + def namespace_icon(namespace, size = 40) + if namespace.kind_of?(Group) + group_icon(namespace.path) + else + avatar_icon(namespace.owner.email, size) + end + end end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index c0c6de0ee7d..ea4b48fdd7f 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -90,4 +90,8 @@ class Namespace < ActiveRecord::Base def kind type == 'Group' ? 'group' : 'user' end + + def find_fork_of(project) + projects.joins(:forked_project_link).where('forked_project_links.forked_from_project_id = ?', project.id).first + end end diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index c2fa1c4bccc..8b9260d661c 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -16,7 +16,7 @@ - unless @project.empty_repo? .fork-buttons - if current_user && can?(current_user, :fork_project, @project) && @project.namespace != current_user.namespace - - if current_user.already_forked?(@project) + - if current_user.already_forked?(@project) && current_user.manageable_namespaces.size < 2 = link_to project_path(current_user.fork_of(@project)), title: 'Go to my fork' do = link_to_toggle_fork - else diff --git a/app/views/projects/forks/new.html.haml b/app/views/projects/forks/new.html.haml index db7486b00e8..54f2cef023b 100644 --- a/app/views/projects/forks/new.html.haml +++ b/app/views/projects/forks/new.html.haml @@ -2,18 +2,37 @@ %p.lead Select namespace where to fork this project %hr -- @namespaces.in_groups_of(6, false) do |group| - .row - - group.each do |namespace| - .col-md-2.col-sm-3 - .thumbnail - = link_to project_fork_path(@project, namespace_id: namespace.id), title: "Fork here", method: "POST" do - - if namespace.kind_of?(Group) - = image_tag group_icon(namespace.path) - - else - = image_tag avatar_icon(namespace.owner.email, 200) - .caption - %h4=namespace.human_name - %p - = namespace.path +.fork-namespaces + - @namespaces.in_groups_of(6, false) do |group| + .row + - group.each do |namespace| + .col-md-2.col-sm-3 + - if fork = namespace.find_fork_of(@project) + .thumbnail.fork-exists-thumbnail + = link_to project_path(fork), title: "Visit project fork", class: 'has_tooltip' do + = image_tag namespace_icon(namespace, 200) + .caption + %h4=namespace.human_name + %p + = namespace.path + - else + .thumbnail.fork-thumbnail + = link_to project_fork_path(@project, namespace_id: namespace.id), title: "Fork here", method: "POST", class: 'has_tooltip' do + = image_tag namespace_icon(namespace, 200) + .caption + %h4=namespace.human_name + %p + = namespace.path + + %p.light + Fork is a copy of a project repository. + %br + Forking a repository allows you to do changes without affecting the original project. + +.save-project-loader.hide + .center + %h2 + %i.fa.fa-spinner.fa-spin + Forking repository + %p Please wait a moment, this page will automatically refresh when ready. diff --git a/features/project/fork.feature b/features/project/fork.feature index d3d1180db04..22f68e5b340 100644 --- a/features/project/fork.feature +++ b/features/project/fork.feature @@ -6,9 +6,11 @@ Feature: Project Fork Scenario: User fork a project Given I click link "Fork" + When I fork to my namespace Then I should see the forked project page Scenario: User already has forked the project Given I already have a project named "Shop" in my namespace And I click link "Fork" + When I fork to my namespace Then I should see a "Name has already been taken" warning diff --git a/features/steps/project/fork.rb b/features/steps/project/fork.rb index da50ba9ced0..8e58597db20 100644 --- a/features/steps/project/fork.rb +++ b/features/steps/project/fork.rb @@ -25,4 +25,10 @@ class Spinach::Features::ProjectFork < Spinach::FeatureSteps step 'I should see a "Name has already been taken" warning' do page.should have_content "Name has already been taken" end + + step 'I fork to my namespace' do + within '.fork-namespaces' do + click_link current_user.name + end + end end