Merge branch '2489-soft-delete-issues' into 'master'

Soft delete issuables

Fixes #2489 

What still needs to happen: research on the indexes, the gem suggests a [lot of changes](https://github.com/rubysherpas/paranoia#about-indexes) though this is probably a good idea to discuss and I'm unsure on the impact of an omnibus upgrade as I suspect creating about 10 new indexes has a large impact on the downtime.

TODO: 
- [x] Also group owners can ***soft*** delete
- [x] Button should be hidden

See merge request !2982

CHANGELOG

@@ -61,6 +61,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

app/controllers/concerns/issuable_actions.rb

+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

app/controllers/projects/issues_controller.rb

 class Projects::IssuesController < Projects::ApplicationController
   include ToggleSubscriptionAction
+  include IssuableActions
 
   before_action :module_enabled
   before_action :issue, only: [:edit, :update, :show]
@@ -133,6 +134,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)

app/controllers/projects/merge_requests_controller.rb

 class Projects::MergeRequestsController < Projects::ApplicationController
   include ToggleSubscriptionAction
   include DiffHelper
+  include IssuableActions
 
   before_action :module_enabled
   before_action :merge_request, only: [
@@ -255,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

app/models/ability.rb

@@ -235,7 +235,9 @@ class Ability
         :rename_project,
         :remove_project,
         :archive_project,
-        :remove_fork_project
+        :remove_fork_project,
+        :destroy_merge_request,
+        :destroy_issue
       ]
     end
 

app/models/concerns/internal_id.rb

@@ -7,7 +7,10 @@ module InternalId
   end
 
   def set_iid
-    max_iid = project.send(self.class.name.tableize).maximum(:iid)
+    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
 

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

app/views/projects/issues/show.html.haml

@@ -45,7 +45,6 @@
       - if can?(current_user, :update_issue, @issue)
         = link_to 'Reopen issue', issue_path(@issue, issue: {state_event: :reopen}, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn btn-nr btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue'
         = link_to 'Close issue', issue_path(@issue, issue: {state_event: :close}, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn btn-nr btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue'
-
         = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'btn btn-nr btn-grouped issuable-edit' do
           = icon('pencil-square-o')
           Edit

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
-          %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'

app/views/shared/issuable/_form.html.haml

@@ -127,7 +127,11 @@
       for this project.
 
   - if issuable.new_record?
-    - cancel_project = issuable.source_project
+    = link_to 'Cancel', namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel'
   - else
-    - cancel_project = issuable.project
-  = link_to 'Cancel', [cancel_project.namespace.becomes(Namespace), cancel_project, issuable], class: 'btn btn-cancel'
+    .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 'Cancel', namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-grouped btn-cancel'

config/routes.rb

@@ -613,7 +613,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :merge_requests, constraints: { id: /\d+/ }, except: [:destroy] do
+        resources :merge_requests, constraints: { id: /\d+/ } do
           member do
             get :commits
             get :diffs
@@ -684,7 +684,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :issues, constraints: { id: /\d+/ }, except: [:destroy] do
+        resources :issues, constraints: { id: /\d+/ } do
           member do
             post :toggle_subscription
           end

db/migrate/20160225090018_add_delete_at_to_issues.rb

+class AddDeleteAtToIssues < ActiveRecord::Migration
+  def change
+    add_column :issues, :deleted_at, :datetime
+    add_index :issues, :deleted_at
+  end
+end

db/migrate/20160225101956_add_delete_at_to_merge_requests.rb

+class AddDeleteAtToMergeRequests < ActiveRecord::Migration
+  def change
+    add_column :merge_requests, :deleted_at, :datetime
+    add_index :merge_requests, :deleted_at
+  end
+end

db/schema.rb

@@ -419,6 +419,7 @@ ActiveRecord::Schema.define(version: 20160320204112) do
     t.integer  "updated_by_id"
     t.integer  "moved_to_id"
     t.boolean  "confidential",              default: false
+    t.datetime "deleted_at"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -426,6 +427,7 @@ ActiveRecord::Schema.define(version: 20160320204112) do
   add_index "issues", ["confidential"], name: "index_issues_on_confidential", using: :btree
   add_index "issues", ["created_at", "id"], name: "index_issues_on_created_at_and_id", using: :btree
   add_index "issues", ["created_at"], name: "index_issues_on_created_at", using: :btree
+  add_index "issues", ["deleted_at"], name: "index_issues_on_deleted_at", using: :btree
   add_index "issues", ["description"], name: "index_issues_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
   add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
@@ -548,12 +550,14 @@ ActiveRecord::Schema.define(version: 20160320204112) do
     t.boolean  "merge_when_build_succeeds", default: false, null: false
     t.integer  "merge_user_id"
     t.string   "merge_commit_sha"
+    t.datetime "deleted_at"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
   add_index "merge_requests", ["author_id"], name: "index_merge_requests_on_author_id", using: :btree
   add_index "merge_requests", ["created_at", "id"], name: "index_merge_requests_on_created_at_and_id", using: :btree
   add_index "merge_requests", ["created_at"], name: "index_merge_requests_on_created_at", using: :btree
+  add_index "merge_requests", ["deleted_at"], name: "index_merge_requests_on_deleted_at", using: :btree
   add_index "merge_requests", ["description"], name: "index_merge_requests_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "merge_requests", ["milestone_id"], name: "index_merge_requests_on_milestone_id", using: :btree
   add_index "merge_requests", ["source_branch"], name: "index_merge_requests_on_source_branch", using: :btree

doc/api/issues.md

@@ -326,17 +326,25 @@ Example response:
 }
 ```
 
-## Delete existing issue (**Deprecated**)
+## Delete an issue
 
-This call is deprecated and returns a `405 Method Not Allowed` error if called.
-An issue gets now closed and is done by calling
-`PUT /projects/:id/issues/:issue_id` with the parameter `state_event` set to
-`close`. See [edit issue](#edit-issue) for more details.
+Only for admins and project owners. Soft deletes the issue in question.
+If the operation is successful, a status code `200` is returned. In case you cannot
+destroy this issue, or it is not present, code `404` is given.
 
 ```
 DELETE /projects/:id/issues/:issue_id
 ```
 
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`            | integer | yes | The ID of a project |
+| `issue_id`      | integer | yes | The ID of a project's issue |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/issues/85
+```
+
 ## Comments on issues
 
 Comments are done via the [notes](notes.md) resource.

doc/api/merge_requests.md

@@ -380,6 +380,25 @@ 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 merge request
+
+Only for admins and project owners. Soft deletes the merge request in question.
+If the operation is successful, a status code `200` is returned. In case you cannot
+destroy this merge request, or it is not present, code `404` is given.
+
+```
+DELETE /projects/:id/merge_requests/:merge_request_id
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id`            | integer | yes | The ID of a project |
+| `merge_request_id` | integer | yes | The ID of a project's merge request |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/merge_request/85
+```
+
 ## Accept MR
 
 Merge changes submitted with MR using this API.

lib/api/helpers.rb

@@ -118,9 +118,7 @@ module API
     end
 
     def authorize!(action, subject)
-      unless abilities.allowed?(current_user, action, subject)
-        forbidden!
-      end
+      forbidden! unless abilities.allowed?(current_user, action, subject)
     end
 
     def authorize_push_project

lib/api/issues.rb

@@ -191,7 +191,7 @@ module API
         end
       end
 
-      # Delete a project issue (deprecated)
+      # Delete a project issue
       #
       # Parameters:
       #   id (required) - The ID of a project
@@ -199,7 +199,10 @@ module API
       # Example Request:
       #   DELETE /projects/:id/issues/:issue_id
       delete ":id/issues/:issue_id" do
-        not_allowed!
+        issue = user_project.issues.find_by(id: params[:issue_id])
+
+        authorize!(:destroy_issue, issue)
+        issue.destroy
       end
     end
   end

lib/api/merge_requests.rb

@@ -100,6 +100,18 @@ module API
         end
       end
 
+      # Delete a MR
+      #
+      # Parameters:
+      # 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_by(id: params[:merge_request_id])
+
+        authorize!(:destroy_merge_request, merge_request)
+        merge_request.destroy
+      end
+
       # Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
       # Use "merge_requests/:merge_request_id/..." instead.
       #

spec/controllers/projects/issues_controller_spec.rb

 require('spec_helper')
 
 describe Projects::IssuesController do
-  describe "GET #index" do
   let(:project) { create(:project_empty_repo) }
   let(:user)    { create(:user) }
   let(:issue)   { create(:issue, project: project) }
 
+  describe "GET #index" do
     before do
       sign_in(user)
       project.team << [user, :developer]
@@ -186,4 +186,29 @@ describe Projects::IssuesController do
       end
     end
   end
+
+  describe "DELETE #destroy" do
+    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 "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 "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(controller).to set_flash[:notice].to(/The issue was successfully deleted\./).now
+      end
+    end
+  end
 end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -157,6 +157,29 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  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)
+    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 "deletes the merge request" do
+        delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid
+
+        expect(response.status).to eq(302)
+        expect(controller).to set_flash[:notice].to(/The merge request was successfully deleted\./).now
+      end
+    end
+  end
+
   describe 'GET diffs' do
     def go(format: 'html')
       get :diffs,

spec/models/issue_spec.rb

@@ -37,6 +37,11 @@ describe Issue, models: true do
 
   subject { create(:issue) }
 
+  describe "act_as_paranoid" do
+    it { is_expected.to have_db_column(:deleted_at) }
+    it { is_expected.to have_db_index(:deleted_at) }
+  end
+
   describe '#to_reference' do
     it 'returns a String reference to the object' do
       expect(subject.to_reference).to eq "##{subject.iid}"

spec/models/merge_request_spec.rb

@@ -49,6 +49,11 @@ describe MergeRequest, models: true do
     it { is_expected.to include_module(Taskable) }
   end
 
+  describe "act_as_paranoid" do
+    it { is_expected.to have_db_column(:deleted_at) }
+    it { is_expected.to have_db_index(:deleted_at) }
+  end
+
   describe 'validation' do
     it { is_expected.to validate_presence_of(:target_branch) }
     it { is_expected.to validate_presence_of(:source_branch) }

spec/requests/api/issues_spec.rb

@@ -6,7 +6,7 @@ describe API::API, api: true  do
   let(:non_member)  { create(:user) }
   let(:author)      { create(:author) }
   let(:assignee)    { create(:assignee) }
-  let(:admin) { create(:admin) }
+  let(:admin)       { create(:user, :admin) }
   let!(:project)    { create(:project, :public, namespace: user.namespace ) }
   let!(:closed_issue) do
     create :closed_issue,
@@ -469,9 +469,25 @@ describe API::API, api: true  do
   end
 
   describe "DELETE /projects/:id/issues/:issue_id" do
-    it "should delete a project issue" do
-      delete api("/projects/#{project.id}/issues/#{issue.id}", user)
-      expect(response.status).to eq(405)
+    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 "rejects a developer from deleting an issue" do
+      delete api("/projects/#{project.id}/issues/#{issue.id}", author)
+      expect(response.status).to be(403)
+    end
+
+    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

spec/requests/api/merge_requests_spec.rb

@@ -4,7 +4,9 @@ describe API::API, api: true  do
   include ApiHelpers
   let(:base_time)   { Time.now }
   let(:user)        { create(:user) }
-  let!(:project) {create(:project, creator_id: user.id, namespace: user.namespace) }
+  let(:admin)       { create(:user, :admin) }
+  let(:non_member)  { create(:user) }
+  let!(:project)    { create(:project, creator_id: user.id, namespace: user.namespace) }
   let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) }
   let!(:merge_request_closed) { create(:merge_request, state: "closed", author: user, assignee: user, source_project: project, target_project: project, title: "Closed test", created_at: base_time + 1.second) }
   let!(:merge_request_merged) { create(:merge_request, state: "merged", author: user, assignee: user, source_project: project, target_project: project, title: "Merged test", created_at: base_time + 2.seconds) }
@@ -315,6 +317,29 @@ describe API::API, api: true  do
     end
   end
 
+  describe "DELETE /projects/:id/merge_requests/:merge_request_id" do
+    context "when the user is developer" do
+      let(:developer) { create(:user) }
+
+      before do
+        project.team << [developer, :developer]
+      end
+
+      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
+
+    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
+
   describe "PUT /projects/:id/merge_requests/:merge_request_id to close MR" do
     it "should return merge_request" do
       put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), state_event: "close"