From 3b088fc5b53f03605484ebef1945b8839abe19de Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <zegerjan@gitlab.com>
Date: Mon, 21 Mar 2016 14:12:52 +0100
Subject: [PATCH] Minor improvements on IssuableActions
---
CHANGELOG | 1 +
app/controllers/concerns/issuable_action.rb | 21 -------
app/controllers/concerns/issuable_actions.rb | 23 ++++++++
app/controllers/projects/issues_controller.rb | 5 +-
.../projects/merge_requests_controller.rb | 5 +-
app/models/ability.rb | 4 +-
.../merge_requests/show/_mr_title.html.haml | 2 +-
app/views/shared/issuable/_form.html.haml | 12 ++--
db/schema.rb | 1 +
doc/api/issues.md | 34 ++---------
doc/api/merge_requests.md | 56 +++----------------
lib/api/issues.rb | 8 +--
lib/api/merge_requests.rb | 6 +-
.../projects/issues_controller_spec.rb | 27 +++++----
.../merge_requests_controller_spec.rb | 24 ++++----
spec/requests/api/issues_spec.rb | 17 ++++--
spec/requests/api/merge_requests_spec.rb | 29 +++++-----
17 files changed, 110 insertions(+), 165 deletions(-)
delete mode 100644 app/controllers/concerns/issuable_action.rb
create mode 100644 app/controllers/concerns/issuable_actions.rb
diff --git a/CHANGELOG b/CHANGELOG
index 3b3ea97c012..78418fd1b94 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -56,6 +56,7 @@ v 8.6.0 (unreleased)
- 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`
- Trigger a todo for mentions on commits page
+ - Let project owners and admins soft delete issues and merge requests
v 8.5.8
- Bump Git version requirement to 2.7.4
diff --git a/app/controllers/concerns/issuable_action.rb b/app/controllers/concerns/issuable_action.rb
deleted file mode 100644
index d82f2bf9ef6..00000000000
--- a/app/controllers/concerns/issuable_action.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-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
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
new file mode 100644
index 00000000000..f40b62446e5
--- /dev/null
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -0,0 +1,23 @@
+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
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 75d48636da8..7c5a597cb6d 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -1,9 +1,9 @@
class Projects::IssuesController < Projects::ApplicationController
include ToggleSubscriptionAction
- include IssuableAction
+ include IssuableActions
before_action :module_enabled
- before_action :issue, only: [:edit, :update, :show, :destroy]
+ before_action :issue, only: [:edit, :update, :show]
# Allow read any issue
before_action :authorize_read_issue!, only: [:show]
@@ -128,6 +128,7 @@ class Projects::IssuesController < Projects::ApplicationController
end
end
alias_method :subscribable_resource, :issue
+ alias_method :issuable, :issue
def authorize_read_issue!
return render_404 unless can?(current_user, :read_issue, @issue)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9cc0c8f64e3..e16acab696c 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -1,11 +1,11 @@
class Projects::MergeRequestsController < Projects::ApplicationController
include ToggleSubscriptionAction
include DiffHelper
- include IssuableAction
+ include IssuableActions
before_action :module_enabled
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
]
before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits, :builds]
@@ -256,6 +256,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@merge_request ||= @project.merge_requests.find_by!(iid: params[:id])
end
alias_method :subscribable_resource, :merge_request
+ alias_method :issuable, :merge_request
def closes_issues
@closes_issues ||= @merge_request.closes_issues
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 8b3d4d4bd29..40c529e8117 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -236,8 +236,8 @@ class Ability
:remove_project,
:archive_project,
:remove_fork_project,
- :remove_merge_request,
- :remove_issue
+ :destroy_merge_request,
+ :destroy_issue
]
end
diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml
index 806e4e83608..ab4b1f14be5 100644
--- a/app/views/projects/merge_requests/show/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/show/_mr_title.html.haml
@@ -29,7 +29,7 @@
- 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 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
- 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'
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 4e1171cfb3d..3457c54bab9 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -114,13 +114,11 @@
for this project.
- if issuable.new_record?
- = link_to namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel' do
- Cancel
+ = link_to 'Cancel', namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel'
- else
- - if current_user.can?(:"remove_#{issuable.to_ability_name}", @project)
- .pull-right
- = link_to polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), method: :delete, class: 'btn' do
+ .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 btn-grouped' do
= icon('trash-o')
Delete
- = link_to namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-cancel' do
- Cancel
+ = link_to 'Cancel', namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-grouped btn-cancel'
diff --git a/db/schema.rb b/db/schema.rb
index 1aa9d435153..a4a17770cd6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -948,6 +948,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
t.string "unlock_token"
t.datetime "otp_grace_period_started_at"
t.boolean "ldap_email", default: false, null: false
+ t.boolean "external", default: false
end
add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
diff --git a/doc/api/issues.md b/doc/api/issues.md
index e2050db06d9..013ad9ffaa2 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -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
@@ -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
```
-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 are done via the [notes](notes.md) resource.
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index 499e56c6395..88c3d251404 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -380,9 +380,12 @@ Parameters:
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.
-## 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
@@ -393,53 +396,8 @@ DELETE /projects/:id/merge_requests/:merge_request_id
| `id` | integer | yes | The ID of a project |
| `merge_request_id` | integer | yes | The ID of a project's merge request |
-Example response:
-
-```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"
-}
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/merge_request/85
```
## Accept MR
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 8c753e9f2ff..e5ae88eb96f 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -199,14 +199,10 @@ module API
# Example Request:
# DELETE /projects/:id/issues/:issue_id
delete ":id/issues/:issue_id" do
- issue = user_project.issues.find(params[:issue_id])
-
- authorize!(:remove_issue, issue)
+ issue = user_project.issues.find_by(id: params[:issue_id])
- issue = user_project.issues.find(params[:issue_id])
+ authorize!(:destroy_issue, issue)
issue.destroy
-
- present issue, with: Entities::Issue
end
end
end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index dc94cc5c85f..93052fba06b 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -106,12 +106,10 @@ module API
# id (required) - The ID of the project
# merge_request_id (required) - The MR id
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
-
- present merge_request, with: Entities::MergeRequest
end
# Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 62643a755b7..86930e317ce 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -1,7 +1,7 @@
require('spec_helper')
describe Projects::IssuesController do
- let(:project) { create(:project) }
+ let(:project) { create(:project) }
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
@@ -188,21 +188,26 @@ describe Projects::IssuesController do
end
describe "DELETE #destroy" do
- it "rejects a developer to destory an issue" do
- delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: issue.iid
- expect(response.status).to eq 404
+ context "when the user is a developer" do
+ before { sign_in(user) }
+ 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
- context "user is an admin" do
- before do
- user.admin = true
- user.save
- end
+ context "when the user is owner" do
+ let(:owner) { create(:user) }
+ let(:namespace) { create(:namespace, owner: owner) }
+ let(:project) { create(:project, namespace: namespace) }
+
+ 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
- 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
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index af154fadb6f..c5b034dc064 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -123,7 +123,7 @@ describe Projects::MergeRequestsController do
end
end
- describe '#index' do
+ describe 'GET #index' do
def get_merge_requests
get :index,
namespace_id: project.namespace.to_param,
@@ -157,23 +157,25 @@ describe Projects::MergeRequestsController do
end
end
- describe "#destroy" do
- it "lets mere mortals not access this endpoint" do
+ describe "DELETE #destroy" 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
- expect(response.status).to eq 404
+ expect(response.status).to eq(404)
end
- context "user is an admin or owner" do
- before do
- user.admin = true
- user.save
- end
+ context "when the user is owner" do
+ let(:owner) { create(:user) }
+ let(:namespace) { create(:namespace, owner: owner) }
+ let(:project) { create(:project, namespace: namespace) }
+
+ 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
- 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
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 6298771925e..ce55cb7b0ae 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -469,20 +469,25 @@ describe API::API, api: true do
end
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)
expect(response.status).to be(403)
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)
expect(response.status).to be(403)
end
- it "deletes the issue if an admin requests it" do
- delete api("/projects/#{project.id}/issues/#{issue.id}", admin)
- expect(response.status).to eq(200)
- expect(json_response['state']).to eq 'opened'
+ context "when the user is project owner" do
+ let(:owner) { create(:user) }
+ let(:project) { create(:project, namespace: owner.namespace) }
+
+ 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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 43e6a59d1ee..ae34684b932 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -317,23 +317,26 @@ describe API::API, api: true do
end
end
- describe "DELETE /projects/:id/merge_request/:merge_request_id" do
- it "owners can destroy" do
- delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user)
+ describe "DELETE /projects/:id/merge_requests/:merge_request_id" do
+ context "when the user is developer" do
+ let(:developer) { create(:user) }
- expect(response.status).to eq(200)
- end
-
- it "let's Admins and owners delete a merge request" do
- delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", admin)
+ before do
+ project.team << [developer, :developer]
+ end
- expect(response.status).to eq(200)
- expect(json_response['id']).to eq merge_request.id
+ it "denies the deletion of the merge request" do
+ delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", developer)
+ expect(response.status).to be(403)
+ end
end
- it "rejects removal from other users" do
- delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", non_member)
- expect(response.status).to eq(404)
+ context "when the user is project owner" do
+ it "destroys the merge request owners can destroy" do
+ delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user)
+
+ expect(response.status).to eq(200)
+ end
end
end
--
GitLab