Dry destroy action on issuables

15 files changed, 56 insertions, 43 deletions

diff --git a/app/controllers/concerns/issuable_action.rb b/app/controllers/concerns/issuable_action.rb
new file mode 100644
index 00000000000..3750a268d7a
--- /dev/null
+++ b/app/controllers/concerns/issuable_action.rb
@@ -0,0 +1,21 @@
+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.gsub('_', ' ')
+
+    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/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 2367ad20574..75d48636da8 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -1,8 +1,9 @@
 class Projects::IssuesController < Projects::ApplicationController
   include ToggleSubscriptionAction
+  include IssuableAction
 
   before_action :module_enabled
-  before_action :issue, only: [:edit, :update, :show]
+  before_action :issue, only: [:edit, :update, :show, :destroy]
 
   # Allow read any issue
   before_action :authorize_read_issue!, only: [:show]
@@ -108,17 +109,6 @@ class Projects::IssuesController < Projects::ApplicationController
     end
   end
 
-  def destroy
-    return access_denied! unless current_user.admin?
-
-    issue.destroy
-
-    respond_to do |format|
-      format.html { redirect_to namespace_project_issues_path(@project.namespace, @project), notice: "The issues was deleted." }
-      format.json { head :ok }
-    end
-  end
-
   def bulk_update
     result = Issues::BulkUpdateService.new(project, current_user, bulk_update_params).execute
     redirect_back_or_default(default: { action: 'index' }, options: { notice: "#{result[:count]} issues updated" })
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 7248ede1699..ff4c9b02cea 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -1,10 +1,11 @@
 class Projects::MergeRequestsController < Projects::ApplicationController
   include ToggleSubscriptionAction
   include DiffHelper
+  include IssuableAction
 
   before_action :module_enabled
   before_action :merge_request, only: [
-    :edit, :update, :show, :diffs, :commits, :builds, :merge, :merge_check,
+    :edit, :update, :show, :destroy, :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]
@@ -164,6 +165,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
   end
 
+<<<<<<< 67043ec53b4c35d5a9862fe78bd3f47e412919cd
   def remove_wip
     MergeRequests::UpdateService.new(project, current_user, title: @merge_request.wipless_title).execute(@merge_request)
 
@@ -171,6 +173,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       notice: "The merge request can now be merged."
   end
 
+=======
+>>>>>>> Dry destroy action on issuables
   def merge_check
     @merge_request.check_if_can_be_merged
 
diff --git a/app/models/ability.rb b/app/models/ability.rb
index e22da4806e6..8b3d4d4bd29 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -235,7 +235,9 @@ class Ability
         :rename_project,
         :remove_project,
         :archive_project,
-        :remove_fork_project
+        :remove_fork_project,
+        :remove_merge_request,
+        :remove_issue
       ]
     end
 
diff --git a/app/models/concerns/internal_id.rb b/app/models/concerns/internal_id.rb
index 3f7be40ed7b..51288094ef1 100644
--- a/app/models/concerns/internal_id.rb
+++ b/app/models/concerns/internal_id.rb
@@ -7,11 +7,10 @@ module InternalId
   end
 
   def set_iid
-    max_iid = if self.paranoid?
-                project.send(self.class.name.tableize).with_deleted.maximum(:iid)
-              else
-                project.send(self.class.name.tableize).maximum(:iid)
-              end
+    records = project.send(self.class.name.tableize)
+    records = records.with_deleted if self.paranoid?
+    max_iid = records.maximum(:iid)
+
     self.iid = max_iid.to_i + 1
   end
 
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 86ab84615ba..31ab5682ad2 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -58,6 +58,8 @@ module Issuable
     attr_mentionable :description, cache: true
     participant :author, :assignee, :notes_with_associations
     strip_attributes :title
+
+    acts_as_paranoid
   end
 
   module ClassMethods
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 4bd91d57ffc..5347d4fa1be 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -54,8 +54,6 @@ class Issue < ActiveRecord::Base
     state :closed
   end
 
-  acts_as_paranoid
-
   def hook_attrs
     attributes
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 5488aa685f1..a015a9ef394 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -142,8 +142,6 @@ class MergeRequest < ActiveRecord::Base
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
 
-  acts_as_paranoid
-
   def self.reference_prefix
     '!'
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 567af921742..c011af03591 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -440,10 +440,6 @@ class User < ActiveRecord::Base
     Project.where("projects.id IN (#{projects_union.to_sql})")
   end
 
-  def owner?(project)
-    owned_projects.include?(project)
-  end
-
   def owned_projects
     @owned_projects ||=
       Project.where('namespace_id IN (?) OR namespace_id = ?',
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 fb05c21648d..806e4e83608 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') #%i.fa.fa-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 6030abe390e..4e1171cfb3d 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -106,10 +106,6 @@
     = f.submit "Submit #{issuable.class.model_name.human.downcase}", class: 'btn btn-create'
   - else
     = f.submit 'Save changes', class: 'btn btn-save'
-    - if current_user.admin? || current_user.owner?(@project)
-      = link_to namespace_project_issue_path(@project.namespace, @project, issuable), method: :delete, class: 'btn' do
-        = icon('trash-o')
-        Delete
 
   - if !issuable.persisted? && !issuable.project.empty_repo? && (guide_url = contribution_guide_path(issuable.project))
     .inline.prepend-left-10
@@ -118,7 +114,13 @@
       for this project.
 
   - if issuable.new_record?
-    - cancel_project = issuable.source_project
+    = link_to namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel' do
+      Cancel
   - else
-    - cancel_project = issuable.project
-  = link_to 'Cancel', [cancel_project.namespace.becomes(Namespace), cancel_project, issuable], class: 'btn btn-cancel'
+    - 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
+          = icon('trash-o')
+          Delete
+    = link_to namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-cancel' do
+      Cancel
diff --git a/doc/api/issues.md b/doc/api/issues.md
index 5df7ce9f006..e2050db06d9 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -328,7 +328,7 @@ Example response:
 
 ## Delete existing issue
 
-Only for admins. Soft deletes the issue in question. Returns the issue which was deleted.
+Only for admins and project owners. Soft deletes the issue in question. Returns the issue which was deleted.
 
 ```
 DELETE /projects/:id/issues/:issue_id
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index 977e16b8601..499e56c6395 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -382,7 +382,7 @@ If an error occurs, an error number and a message explaining the reason is retur
 
 ## Delete a MR
 
-Soft deletes a merge request. For admins only.
+Soft deletes a merge request. For admins and owners only.
 
 ```
 DELETE /projects/:id/merge_requests/:merge_request_id
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 5b47bbc0b1b..539f00d37fe 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -199,11 +199,12 @@ module API
       # Example Request:
       #   DELETE /projects/:id/issues/:issue_id
       delete ":id/issues/:issue_id" do
-        authenticated_as_admin!
+        issue = user_project.issues.find(params[:issue_id])
+        !JLJsdf sdfijsf current_user.can?(:remove_issue, issue)
 
         issue = user_project.issues.find(params[:issue_id])
         issue.destroy
-        
+
         present issue, with: Entities::Issue
       end
     end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 6c1aa8368ea..99d1e1c595e 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 'GET #index' do
+  describe '#index' do
     def get_merge_requests
       get :index,
           namespace_id: project.namespace.to_param,
@@ -157,8 +157,8 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  describe "DELETE #destroy" do
-    it "lets mere mortals not acces this endpoint" do
+  describe "#destroy" do
+    it "lets mere mortals not access this endpoint" do
       delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
 
       expect(response.status).to eq 404
@@ -170,7 +170,7 @@ describe Projects::MergeRequestsController do
         user.save
       end
 
-      it "lets an admin delete an issue" do
+      it "lets an admin or owner delete an issue" do
         delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
 
         expect(response.status).to be 302