Minor improvements on IssuableActions

17 files changed, 110 insertions, 165 deletions

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