Commit 3b088fc5 authored by Zeger-Jan van de Weg's avatar Zeger-Jan van de Weg

Minor improvements on IssuableActions

parent 98fd60f5
No related merge requests found
...@@ -56,6 +56,7 @@ v 8.6.0 (unreleased) ...@@ -56,6 +56,7 @@ v 8.6.0 (unreleased)
- User deletion is now done in the background so the request can not time out - User deletion is now done in the background so the request can not time out
- Canceled builds are now ignored in compound build status if marked as `allowed to fail` - Canceled builds are now ignored in compound build status if marked as `allowed to fail`
- Trigger a todo for mentions on commits page - Trigger a todo for mentions on commits page
- Let project owners and admins soft delete issues and merge requests
v 8.5.8 v 8.5.8
- Bump Git version requirement to 2.7.4 - Bump Git version requirement to 2.7.4
......
module IssuableAction
extend ActiveSupport::Concern
def destroy
issuable = @merge_request || @issue
unless current_user.can?(:"remove_#{issuable.to_ability_name}", issuable)
return access_denied!
end
issuable.destroy
route = polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class])
issuable_name = issuable.class.name.underscore.tr('_', ' ')
respond_to do |format|
format.html { redirect_to route, notice: "This #{issuable_name} was deleted." }
format.json { head :ok }
end
end
end
module IssuableActions
extend ActiveSupport::Concern
included do
before_action :authorize_destroy_issuable!, only: :destroy
end
def destroy
issuable.destroy
name = issuable.class.name.titleize.downcase
flash[:notice] = "The #{name} was successfully deleted."
redirect_to polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class])
end
private
def authorize_destroy_issuable!
unless current_user.can?(:"destroy_#{issuable.to_ability_name}", issuable)
return access_denied!
end
end
end
class Projects::IssuesController < Projects::ApplicationController class Projects::IssuesController < Projects::ApplicationController
include ToggleSubscriptionAction include ToggleSubscriptionAction
include IssuableAction include IssuableActions
before_action :module_enabled before_action :module_enabled
before_action :issue, only: [:edit, :update, :show, :destroy] before_action :issue, only: [:edit, :update, :show]
# Allow read any issue # Allow read any issue
before_action :authorize_read_issue!, only: [:show] before_action :authorize_read_issue!, only: [:show]
...@@ -128,6 +128,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -128,6 +128,7 @@ class Projects::IssuesController < Projects::ApplicationController
end end
end end
alias_method :subscribable_resource, :issue alias_method :subscribable_resource, :issue
alias_method :issuable, :issue
def authorize_read_issue! def authorize_read_issue!
return render_404 unless can?(current_user, :read_issue, @issue) return render_404 unless can?(current_user, :read_issue, @issue)
......
class Projects::MergeRequestsController < Projects::ApplicationController class Projects::MergeRequestsController < Projects::ApplicationController
include ToggleSubscriptionAction include ToggleSubscriptionAction
include DiffHelper include DiffHelper
include IssuableAction include IssuableActions
before_action :module_enabled before_action :module_enabled
before_action :merge_request, only: [ before_action :merge_request, only: [
:edit, :update, :show, :destroy, :diffs, :commits, :builds, :merge, :merge_check, :edit, :update, :show, :diffs, :commits, :builds, :merge, :merge_check,
:ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip
] ]
before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds] before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds]
...@@ -256,6 +256,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -256,6 +256,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@merge_request ||= @project.merge_requests.find_by!(iid: params[:id]) @merge_request ||= @project.merge_requests.find_by!(iid: params[:id])
end end
alias_method :subscribable_resource, :merge_request alias_method :subscribable_resource, :merge_request
alias_method :issuable, :merge_request
def closes_issues def closes_issues
@closes_issues ||= @merge_request.closes_issues @closes_issues ||= @merge_request.closes_issues
......
...@@ -236,8 +236,8 @@ class Ability ...@@ -236,8 +236,8 @@ class Ability
:remove_project, :remove_project,
:archive_project, :archive_project,
:remove_fork_project, :remove_fork_project,
:remove_merge_request, :destroy_merge_request,
:remove_issue :destroy_issue
] ]
end end
......
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,7 @@
- if @merge_request.open? - if @merge_request.open?
= link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: 'btn btn-nr btn-grouped btn-close', title: 'Close merge request' = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: 'btn btn-nr btn-grouped btn-close', title: 'Close merge request'
= link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-nr btn-grouped issuable-edit', id: 'edit_merge_request' do = link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-nr btn-grouped issuable-edit', id: 'edit_merge_request' do
=icon('pencil-square-o') = icon('pencil-square-o')
Edit Edit
- if @merge_request.closed? - if @merge_request.closed?
= link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'btn btn-nr btn-grouped btn-reopen reopen-mr-link', title: 'Reopen merge request' = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'btn btn-nr btn-grouped btn-reopen reopen-mr-link', title: 'Reopen merge request'
...@@ -114,13 +114,11 @@ ...@@ -114,13 +114,11 @@
for this project. for this project.
- if issuable.new_record? - if issuable.new_record?
= link_to namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel' do = link_to 'Cancel', namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel'
Cancel
- else - else
- if current_user.can?(:"remove_#{issuable.to_ability_name}", @project) .pull-right
.pull-right - if current_user.can?(:"destroy_#{issuable.to_ability_name}", @project)
= link_to polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), method: :delete, class: 'btn' do = link_to polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), method: :delete, class: 'btn btn-grouped' do
= icon('trash-o') = icon('trash-o')
Delete Delete
= link_to namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-cancel' do = link_to 'Cancel', namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-grouped btn-cancel'
Cancel
...@@ -948,6 +948,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do ...@@ -948,6 +948,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
t.string "unlock_token" t.string "unlock_token"
t.datetime "otp_grace_period_started_at" t.datetime "otp_grace_period_started_at"
t.boolean "ldap_email", default: false, null: false t.boolean "ldap_email", default: false, null: false
t.boolean "external", default: false
end end
add_index "users", ["admin"], name: "index_users_on_admin", using: :btree add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
......
...@@ -326,9 +326,11 @@ Example response: ...@@ -326,9 +326,11 @@ Example response:
} }
``` ```
## Delete existing issue ## Delete an issue
Only for admins and project owners. Soft deletes the issue in question. Returns the issue which was deleted. Only for admins and project owners. Soft deletes the issue in question.
If the operation is successful, a status code of `200` is returned. Any the case you cannot
destroy this issue, or it is not present, code `404` is given.
``` ```
DELETE /projects/:id/issues/:issue_id DELETE /projects/:id/issues/:issue_id
...@@ -343,34 +345,6 @@ DELETE /projects/:id/issues/:issue_id ...@@ -343,34 +345,6 @@ DELETE /projects/:id/issues/:issue_id
curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/issues/85 curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/issues/85
``` ```
Example response:
```json
{
"created_at" : "2016-01-07T12:46:01.410Z",
"author" : {
"name" : "Alexandra Bashirian",
"avatar_url" : null,
"username" : "eileen.lowe",
"id" : 18,
"state" : "active",
"web_url" : "https://gitlab.example.com/u/eileen.lowe"
},
"state" : "closed",
"title" : "Issues with auth",
"project_id" : 4,
"description" : null,
"updated_at" : "2016-01-07T12:55:16.213Z",
"iid" : 15,
"labels" : [
"bug"
],
"id" : 85,
"assignee" : null,
"milestone" : null
}
```
## Comments on issues ## Comments on issues
Comments are done via the [notes](notes.md) resource. Comments are done via the [notes](notes.md) resource.
...@@ -380,9 +380,12 @@ Parameters: ...@@ -380,9 +380,12 @@ Parameters:
If the operation is successful, 200 and the updated merge request is returned. If the operation is successful, 200 and the updated merge request is returned.
If an error occurs, an error number and a message explaining the reason is returned. If an error occurs, an error number and a message explaining the reason is returned.
## Delete a MR ## Delete a merge request
Only for admins and project owners. Soft deletes the merge request in question.
If the operation is successful, a status code of `200` is returned. Any the case you cannot
destroy this merge request, or it is not present, code `404` is given.
Soft deletes a merge request. For admins and owners only.
``` ```
DELETE /projects/:id/merge_requests/:merge_request_id DELETE /projects/:id/merge_requests/:merge_request_id
...@@ -393,53 +396,8 @@ DELETE /projects/:id/merge_requests/:merge_request_id ...@@ -393,53 +396,8 @@ DELETE /projects/:id/merge_requests/:merge_request_id
| `id` | integer | yes | The ID of a project | | `id` | integer | yes | The ID of a project |
| `merge_request_id` | integer | yes | The ID of a project's merge request | | `merge_request_id` | integer | yes | The ID of a project's merge request |
Example response: ```bash
curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/merge_request/85
```json
{
"id": 1,
"target_branch": "master",
"source_branch": "test1",
"project_id": 3,
"title": "test1",
"state": "merged",
"upvotes": 0,
"downvotes": 0,
"author": {
"id": 1,
"username": "admin",
"email": "admin@example.com",
"name": "Administrator",
"state": "active",
"created_at": "2012-04-29T08:46:00Z"
},
"assignee": {
"id": 1,
"username": "admin",
"email": "admin@example.com",
"name": "Administrator",
"state": "active",
"created_at": "2012-04-29T08:46:00Z"
},
"source_project_id": 4,
"target_project_id": 4,
"labels": [ ],
"description":"fixed login page css paddings",
"work_in_progress": false,
"milestone": {
"id": 5,
"iid": 1,
"project_id": 4,
"title": "v2.0",
"description": "Assumenda aut placeat expedita exercitationem labore sunt enim earum.",
"state": "closed",
"created_at": "2015-02-02T19:49:26.013Z",
"updated_at": "2015-02-02T19:49:26.013Z",
"due_date": null
},
"merge_when_build_succeeds": true,
"merge_status": "can_be_merged"
}
``` ```
## Accept MR ## Accept MR
......
...@@ -199,14 +199,10 @@ module API ...@@ -199,14 +199,10 @@ module API
# Example Request: # Example Request:
# DELETE /projects/:id/issues/:issue_id # DELETE /projects/:id/issues/:issue_id
delete ":id/issues/:issue_id" do delete ":id/issues/:issue_id" do
issue = user_project.issues.find(params[:issue_id]) issue = user_project.issues.find_by(id: params[:issue_id])
authorize!(:remove_issue, issue)
issue = user_project.issues.find(params[:issue_id]) authorize!(:destroy_issue, issue)
issue.destroy issue.destroy
present issue, with: Entities::Issue
end end
end end
end end
......
...@@ -106,12 +106,10 @@ module API ...@@ -106,12 +106,10 @@ module API
# id (required) - The ID of the project # id (required) - The ID of the project
# merge_request_id (required) - The MR id # merge_request_id (required) - The MR id
delete ":id/merge_requests/:merge_request_id" do delete ":id/merge_requests/:merge_request_id" do
merge_request = user_project.merge_requests.find(params[:merge_request_id]) merge_request = user_project.merge_requests.find_by(id: params[:merge_request_id])
authorize!(:remove_merge_request, merge_request) authorize!(:destroy_merge_request, merge_request)
merge_request.destroy merge_request.destroy
present merge_request, with: Entities::MergeRequest
end end
# Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0 # Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
......
require('spec_helper') require('spec_helper')
describe Projects::IssuesController do describe Projects::IssuesController do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
...@@ -188,21 +188,26 @@ describe Projects::IssuesController do ...@@ -188,21 +188,26 @@ describe Projects::IssuesController do
end end
describe "DELETE #destroy" do describe "DELETE #destroy" do
it "rejects a developer to destory an issue" do context "when the user is a developer" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: issue.iid before { sign_in(user) }
expect(response.status).to eq 404 it "rejects a developer to destroy an issue" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: issue.iid
expect(response.status).to eq(404)
end
end end
context "user is an admin" do context "when the user is owner" do
before do let(:owner) { create(:user) }
user.admin = true let(:namespace) { create(:namespace, owner: owner) }
user.save let(:project) { create(:project, namespace: namespace) }
end
before { sign_in owner }
it "lets an admin delete an issue" do it "deletes the issue" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: issue.iid delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: issue.iid
expect(response.status).to eq 302 expect(response.status).to eq(302)
expect(controller).to set_flash[:notice].to(/The issue was successfully deleted\./).now
end end
end end
end end
......
...@@ -123,7 +123,7 @@ describe Projects::MergeRequestsController do ...@@ -123,7 +123,7 @@ describe Projects::MergeRequestsController do
end end
end end
describe '#index' do describe 'GET #index' do
def get_merge_requests def get_merge_requests
get :index, get :index,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
...@@ -157,23 +157,25 @@ describe Projects::MergeRequestsController do ...@@ -157,23 +157,25 @@ describe Projects::MergeRequestsController do
end end
end end
describe "#destroy" do describe "DELETE #destroy" do
it "lets mere mortals not access this endpoint" do it "denies access to users unless they're admin or project owner" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
expect(response.status).to eq 404 expect(response.status).to eq(404)
end end
context "user is an admin or owner" do context "when the user is owner" do
before do let(:owner) { create(:user) }
user.admin = true let(:namespace) { create(:namespace, owner: owner) }
user.save let(:project) { create(:project, namespace: namespace) }
end
before { sign_in owner }
it "lets an admin or owner delete an issue" do it "deletes the merge request" do
delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
expect(response.status).to be 302 expect(response.status).to eq(302)
expect(controller).to set_flash[:notice].to(/The merge request was successfully deleted\./).now
end end
end end
end end
......
...@@ -469,20 +469,25 @@ describe API::API, api: true do ...@@ -469,20 +469,25 @@ describe API::API, api: true do
end end
describe "DELETE /projects/:id/issues/:issue_id" do describe "DELETE /projects/:id/issues/:issue_id" do
it "should reject a non member from deleting an issue" do it "rejects a non member from deleting an issue" do
delete api("/projects/#{project.id}/issues/#{issue.id}", non_member) delete api("/projects/#{project.id}/issues/#{issue.id}", non_member)
expect(response.status).to be(403) expect(response.status).to be(403)
end end
it "should reject a developer from deleting an issue" do it "rejects a developer from deleting an issue" do
delete api("/projects/#{project.id}/issues/#{issue.id}", author) delete api("/projects/#{project.id}/issues/#{issue.id}", author)
expect(response.status).to be(403) expect(response.status).to be(403)
end end
it "deletes the issue if an admin requests it" do context "when the user is project owner" do
delete api("/projects/#{project.id}/issues/#{issue.id}", admin) let(:owner) { create(:user) }
expect(response.status).to eq(200) let(:project) { create(:project, namespace: owner.namespace) }
expect(json_response['state']).to eq 'opened'
it "deletes the issue if an admin requests it" do
delete api("/projects/#{project.id}/issues/#{issue.id}", owner)
expect(response.status).to eq(200)
expect(json_response['state']).to eq 'opened'
end
end end
end end
end end
...@@ -317,23 +317,26 @@ describe API::API, api: true do ...@@ -317,23 +317,26 @@ describe API::API, api: true do
end end
end end
describe "DELETE /projects/:id/merge_request/:merge_request_id" do describe "DELETE /projects/:id/merge_requests/:merge_request_id" do
it "owners can destroy" do context "when the user is developer" do
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user) let(:developer) { create(:user) }
expect(response.status).to eq(200) before do
end project.team << [developer, :developer]
end
it "let's Admins and owners delete a merge request" do
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", admin)
expect(response.status).to eq(200) it "denies the deletion of the merge request" do
expect(json_response['id']).to eq merge_request.id delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", developer)
expect(response.status).to be(403)
end
end end
it "rejects removal from other users" do context "when the user is project owner" do
delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", non_member) it "destroys the merge request owners can destroy" do
expect(response.status).to eq(404) delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user)
expect(response.status).to eq(200)
end
end end
end end
......
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