Commit 0bea5ced authored by Han Loong Liauw's avatar Han Loong Liauw

Made suggested content changes based on MR Review

Changed the authentication method for removing fork through API
Reflected changes to new auth method in API specs
parent 520d8509
...@@ -46,7 +46,7 @@ v 8.1.0 (unreleased) ...@@ -46,7 +46,7 @@ v 8.1.0 (unreleased)
- Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji) - Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji)
- Persist filters when sorting on admin user page (Jerry Lukins) - Persist filters when sorting on admin user page (Jerry Lukins)
- Adds ability to remove the forked relationship from project settings - Adds ability to remove the forked relationship from project settings
screen. #2578 (Han Loong Liauw) screen. (Han Loong Liauw)
- Add spellcheck=false to certain input fields - Add spellcheck=false to certain input fields
- Invalidate stored service password if the endpoint URL is changed - Invalidate stored service password if the endpoint URL is changed
......
...@@ -5,7 +5,7 @@ class ProjectsController < ApplicationController ...@@ -5,7 +5,7 @@ class ProjectsController < ApplicationController
before_action :repository, except: [:new, :create] before_action :repository, except: [:new, :create]
# Authorize # Authorize
before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive, :remove_fork] before_action :authorize_admin_project!, only: [:edit, :update]
before_action :event_filter, only: [:show, :activity] before_action :event_filter, only: [:show, :activity]
layout :determine_layout layout :determine_layout
...@@ -56,6 +56,8 @@ class ProjectsController < ApplicationController ...@@ -56,6 +56,8 @@ class ProjectsController < ApplicationController
end end
def transfer def transfer
return access_denied! unless can?(current_user, :change_namespace, @project)
namespace = Namespace.find_by(id: params[:new_namespace_id]) namespace = Namespace.find_by(id: params[:new_namespace_id])
::Projects::TransferService.new(project, current_user).execute(namespace) ::Projects::TransferService.new(project, current_user).execute(namespace)
...@@ -65,6 +67,8 @@ class ProjectsController < ApplicationController ...@@ -65,6 +67,8 @@ class ProjectsController < ApplicationController
end end
def remove_fork def remove_fork
return access_denied! unless can?(current_user, :remove_fork_project, @project)
if @project.forked? if @project.forked?
@project.forked_project_link.destroy @project.forked_project_link.destroy
flash[:notice] = 'Fork relationship has been removed.' flash[:notice] = 'Fork relationship has been removed.'
......
...@@ -190,17 +190,17 @@ ...@@ -190,17 +190,17 @@
.nothing-here-block Only the project owner can transfer a project .nothing-here-block Only the project owner can transfer a project
- if @project.forked? && can?(current_user, :remove_fork_project, @project) - if @project.forked? && can?(current_user, :remove_fork_project, @project)
= form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :put, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f| = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :delete, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f|
.panel.panel-default.panel.panel-danger .panel.panel-default.panel.panel-danger
.panel-heading Remove forked relationship .panel-heading Remove fork relationship
.panel-body .panel-body
%p %p
This will remove the relationship to the source project from This will remove the relationship to the source project from
= link_to project_path(@project.forked_from_project) do = link_to project_path(@project.forked_from_project) do
= @project.forked_from_project.namespace.try(:name) = @project.forked_from_project.namespace.try(:name)
%br %br
%strong Once removed it cannot be reversed through this interface %strong Once removed it cannot be reversed through this interface.
= button_to 'Remove forked relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } = button_to 'Remove fork relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) }
- elsif @project.forked? - elsif @project.forked?
.nothing-here-block Only the project owner can remove the fork relationship .nothing-here-block Only the project owner can remove the fork relationship
......
...@@ -378,7 +378,7 @@ Gitlab::Application.routes.draw do ...@@ -378,7 +378,7 @@ Gitlab::Application.routes.draw do
[:new, :create, :index], path: "/") do [:new, :create, :index], path: "/") do
member do member do
put :transfer put :transfer
put :remove_fork delete :remove_fork
post :archive post :archive
post :unarchive post :unarchive
post :toggle_star post :toggle_star
......
...@@ -246,7 +246,7 @@ module API ...@@ -246,7 +246,7 @@ module API
# Example Request: # Example Request:
# DELETE /projects/:id/fork # DELETE /projects/:id/fork
delete ":id/fork" do delete ":id/fork" do
authenticated_as_admin! authorize! :remove_fork_project, user_project
if user_project.forked? if user_project.forked?
user_project.forked_project_link.destroy user_project.forked_project_link.destroy
end end
......
...@@ -72,9 +72,12 @@ describe ProjectsController do ...@@ -72,9 +72,12 @@ describe ProjectsController do
context 'with forked project' do context 'with forked project' do
let(:project_fork) { create(:project, namespace: user.namespace) } let(:project_fork) { create(:project, namespace: user.namespace) }
it 'should remove fork from project' do before do
create(:forked_project_link, forked_to_project: project_fork) create(:forked_project_link, forked_to_project: project_fork)
put(:remove_fork, end
it 'should remove fork from project' do
delete(:remove_fork,
namespace_id: project_fork.namespace.to_param, namespace_id: project_fork.namespace.to_param,
id: project_fork.to_param, format: :js) id: project_fork.to_param, format: :js)
...@@ -84,9 +87,11 @@ describe ProjectsController do ...@@ -84,9 +87,11 @@ describe ProjectsController do
end end
end end
context 'when project not forked' do
let(:unforked_project) { create(:project, namespace: user.namespace) }
it 'should do nothing if project was not forked' do it 'should do nothing if project was not forked' do
unforked_project = create(:project, namespace: user.namespace) delete(:remove_fork,
put(:remove_fork,
namespace_id: unforked_project.namespace.to_param, namespace_id: unforked_project.namespace.to_param,
id: unforked_project.to_param, format: :js) id: unforked_project.to_param, format: :js)
...@@ -94,9 +99,10 @@ describe ProjectsController do ...@@ -94,9 +99,10 @@ describe ProjectsController do
expect(response).to render_template(:remove_fork) expect(response).to render_template(:remove_fork)
end end
end end
end
it "does nothing if user is not signed in" do it "does nothing if user is not signed in" do
put(:remove_fork, delete(:remove_fork,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
id: project.to_param, format: :js) id: project.to_param, format: :js)
expect(response.status).to eq(401) expect(response.status).to eq(401)
......
...@@ -45,13 +45,13 @@ feature 'Project', feature: true do ...@@ -45,13 +45,13 @@ feature 'Project', feature: true do
end end
it 'should remove fork' do it 'should remove fork' do
expect(page).to have_content 'Remove forked relationship' expect(page).to have_content 'Remove fork relationship'
remove_with_confirm('Remove forked relationship', project.path) remove_with_confirm('Remove fork relationship', project.path)
expect(page).to have_content 'Fork relationship has been removed.' expect(page).to have_content 'Fork relationship has been removed.'
expect(project.forked?).to be_falsey expect(project.forked?).to be_falsey
expect(page).not_to have_content 'Remove forked relationship' expect(page).not_to have_content 'Remove fork relationship'
end end
end end
......
...@@ -606,8 +606,21 @@ describe API::API, api: true do ...@@ -606,8 +606,21 @@ describe API::API, api: true do
describe 'DELETE /projects/:id/fork' do describe 'DELETE /projects/:id/fork' do
it "shouldn't available for non admin users" do it "shouldn't be visible to users outside group" do
delete api("/projects/#{project_fork_target.id}/fork", user) delete api("/projects/#{project_fork_target.id}/fork", user)
expect(response.status).to eq(404)
end
context 'when users belong to project group' do
let(:project_fork_target) { create(:project, group: create(:group)) }
before do
project_fork_target.group.add_owner user
project_fork_target.group.add_developer user2
end
it 'should be forbidden to non-owner users' do
delete api("/projects/#{project_fork_target.id}/fork", user2)
expect(response.status).to eq(403) expect(response.status).to eq(403)
end end
...@@ -631,6 +644,7 @@ describe API::API, api: true do ...@@ -631,6 +644,7 @@ describe API::API, api: true do
end end
end end
end end
end
describe 'GET /projects/search/:query' do describe 'GET /projects/search/:query' do
let!(:query) { 'query'} let!(:query) { 'query'}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment