diff options
author | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-14 21:46:44 +0100 |
---|---|---|
committer | Zeger-Jan van de Weg <zegerjan@gitlab.com> | 2016-03-19 21:24:05 +0100 |
commit | 7342a4566cc2eef0e434f3aea0eac48674baaaf1 (patch) | |
tree | 2c01812a3736544d759dd4cdd59cd0a6ae1bb408 | |
parent | 67043ec53b4c35d5a9862fe78bd3f47e412919cd (diff) | |
download | gitlab-ce-7342a4566cc2eef0e434f3aea0eac48674baaaf1.tar.gz |
Dry destroy action on issuables
-rw-r--r-- | app/controllers/concerns/issuable_action.rb | 21 | ||||
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 14 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 6 | ||||
-rw-r--r-- | app/models/ability.rb | 4 | ||||
-rw-r--r-- | app/models/concerns/internal_id.rb | 9 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 2 | ||||
-rw-r--r-- | app/models/issue.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 4 | ||||
-rw-r--r-- | app/views/projects/merge_requests/show/_mr_title.html.haml | 2 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 16 | ||||
-rw-r--r-- | doc/api/issues.md | 2 | ||||
-rw-r--r-- | doc/api/merge_requests.md | 2 | ||||
-rw-r--r-- | lib/api/issues.rb | 5 | ||||
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 8 |
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 |