kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

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

Browse source 
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
This commit is contained in:
Douwe Maan 2016-03-21 19:28:48 +00:00
commit 4a16802615
25 changed files with 231 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG
View file

@ -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

23
app/controllers/concerns/issuable_actions.rb Normal file
View file

@ -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

2
app/controllers/projects/issues_controller.rb
View file

@ -1,5 +1,6 @@
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)

2
app/controllers/projects/merge_requests_controller.rb
View file

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

4
app/models/ability.rb
View file

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

5
app/models/concerns/internal_id.rb
View file

@ -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

2
app/models/concerns/issuable.rb
View file

@ -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

1
app/views/projects/issues/show.html.haml
View file

@ -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

2
app/views/projects/merge_requests/show/_mr_title.html.haml
View file

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

10
app/views/shared/issuable/_form.html.haml
View file

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

4
config/routes.rb
View file

@ -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

6
db/migrate/20160225090018_add_delete_at_to_issues.rb Normal file
View file

@ -0,0 +1,6 @@
class AddDeleteAtToIssues < ActiveRecord::Migration
def change
add_column :issues, :deleted_at, :datetime
add_index :issues, :deleted_at
end
end

6
db/migrate/20160225101956_add_delete_at_to_merge_requests.rb Normal file
View file

@ -0,0 +1,6 @@
class AddDeleteAtToMergeRequests < ActiveRecord::Migration
def change
add_column :merge_requests, :deleted_at, :datetime
add_index :merge_requests, :deleted_at
end
end

4
db/schema.rb
View file

@ -418,6 +418,7 @@ ActiveRecord::Schema.define(version: 20160317092222) 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
@ -425,6 +426,7 @@ ActiveRecord::Schema.define(version: 20160317092222) 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
@ -547,12 +549,14 @@ ActiveRecord::Schema.define(version: 20160317092222) 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

18
doc/api/issues.md
View file

@ -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.

19
doc/api/merge_requests.md
View file

@ -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.

4
lib/api/helpers.rb
View file

@ -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

7
lib/api/issues.rb
View file

@ -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

12
lib/api/merge_requests.rb
View file

@ -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.
#

33
spec/controllers/projects/issues_controller_spec.rb
View file

@ -1,11 +1,11 @@
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) }
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

23
spec/controllers/projects/merge_requests_controller_spec.rb
View file

@ -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,

5
spec/models/issue_spec.rb
View file

@ -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}"

5
spec/models/merge_request_spec.rb
View file

@ -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) }

34
spec/requests/api/issues_spec.rb
View file

@ -2,12 +2,12 @@ require 'spec_helper'
describe API::API, api: true do
include ApiHelpers
let(:user) { create(:user) }
let(:non_member) { create(:user) }
let(:author) { create(:author) }
let(:assignee) { create(:assignee) }
let(:admin) { create(:admin) }
let!(:project) { create(:project, :public, namespace: user.namespace ) }
let(:user) { create(:user) }
let(:non_member) { create(:user) }
let(:author) { create(:author) }
let(:assignee) { create(:assignee) }
let(:admin) { create(:user, :admin) }
let!(:project) { create(:project, :public, namespace: user.namespace ) }
let!(:closed_issue) do
create :closed_issue,
author: user,
@ -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

37
spec/requests/api/merge_requests_spec.rb
View file

@ -2,15 +2,17 @@ require "spec_helper"
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(:base_time) { Time.now }
let(:user) { create(:user) }
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) }
let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
let!(:note2) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "another comment on a MR") }
let(:milestone) { create(:milestone, title: '1.0.0', project: project) }
let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
let!(:note2) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "another comment on a MR") }
let(:milestone) { create(:milestone, title: '1.0.0', project: project) }
before do
project.team << [user, :reporters]
@ -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"